[RFC,v2,02/16] apps: common: event_loop: Disable copy/move
diff mbox series

Message ID 20250114182143.1773762-3-pobrn@protonmail.com
State Superseded
Headers show
Series
  • apps: lc-compliance: Multi-stream tests
Related show

Commit Message

Barnabás Pőcze Jan. 14, 2025, 6:21 p.m. UTC
The compiler generated functions are not appropriate, so
delete the copy/move constructor/assignment to avoid
potential issues.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---
 src/apps/common/event_loop.h | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Jacopo Mondi Jan. 22, 2025, 9:03 a.m. UTC | #1
Hi Barnabás

On Tue, Jan 14, 2025 at 06:21:56PM +0000, Barnabás Pőcze wrote:
> The compiler generated functions are not appropriate, so
> delete the copy/move constructor/assignment to avoid
> potential issues.
>
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>

The class has this member
	std::list<std::unique_ptr<Event>> events_;

Is there a copy constructor generated for the class even if
unique_ptr<> do not provide one ?

> ---
>  src/apps/common/event_loop.h | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/src/apps/common/event_loop.h b/src/apps/common/event_loop.h
> index d7d012c76..4e8dd0a46 100644
> --- a/src/apps/common/event_loop.h
> +++ b/src/apps/common/event_loop.h
> @@ -13,6 +13,8 @@
>  #include <memory>
>  #include <mutex>
>
> +#include <libcamera/base/class.h>
> +
>  #include <event2/util.h>
>
>  struct event_base;
> @@ -43,8 +45,11 @@ public:
>  			   std::function<void()> &&handler);
>
>  private:
> +	LIBCAMERA_DISABLE_COPY_AND_MOVE(EventLoop)
> +
>  	struct Event {
>  		Event(std::function<void()> &&callback);
> +		LIBCAMERA_DISABLE_COPY_AND_MOVE(Event)
>  		~Event();
>
>  		static void dispatch(int fd, short events, void *arg);
> --
> 2.48.0
>
>
Barnabás Pőcze Jan. 24, 2025, 9:23 a.m. UTC | #2
Hi


2025. január 22., szerda 10:03 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:

> Hi Barnabás
> 
> On Tue, Jan 14, 2025 at 06:21:56PM +0000, Barnabás Pőcze wrote:
> > The compiler generated functions are not appropriate, so
> > delete the copy/move constructor/assignment to avoid
> > potential issues.
> >
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> 
> The class has this member
> 	std::list<std::unique_ptr<Event>> events_;
> 
> Is there a copy constructor generated for the class even if
> unique_ptr<> do not provide one ?

That list would indeed cause the copy constructor/assignment to be deleted,
however, move constructor/assignment would still be generated, which would
be incorrect since the owning `event_base` pointer is stored in a raw pointer.
However, the class has an `std::mutex` member, so there are in fact no copy/move
constructors/assignment operators defined by default.

So this change does not really change much, my bad, this can be probably be ignored.
Nonetheless, I have run into at least one situation in libcamera where a move
constructor was defined by the compiler but semantically it was wrong. Specifically,
it was `EventNotifier`; I am wondering if `Object` should disable copy/move altogether.

In any case, I'd support it if libcamera would enforce the *rule of 0/5* more strictly.
( https://en.cppreference.com/w/cpp/language/rule_of_three )


Regards,
Barnabás Pőcze

> 
> > ---
> >  src/apps/common/event_loop.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/src/apps/common/event_loop.h b/src/apps/common/event_loop.h
> > index d7d012c76..4e8dd0a46 100644
> > --- a/src/apps/common/event_loop.h
> > +++ b/src/apps/common/event_loop.h
> > @@ -13,6 +13,8 @@
> >  #include <memory>
> >  #include <mutex>
> >
> > +#include <libcamera/base/class.h>
> > +
> >  #include <event2/util.h>
> >
> >  struct event_base;
> > @@ -43,8 +45,11 @@ public:
> >  			   std::function<void()> &&handler);
> >
> >  private:
> > +	LIBCAMERA_DISABLE_COPY_AND_MOVE(EventLoop)
> > +
> >  	struct Event {
> >  		Event(std::function<void()> &&callback);
> > +		LIBCAMERA_DISABLE_COPY_AND_MOVE(Event)
> >  		~Event();
> >
> >  		static void dispatch(int fd, short events, void *arg);
> > --
> > 2.48.0
> >
> >
>
Jacopo Mondi Jan. 24, 2025, 11:17 a.m. UTC | #3
Hi Barnabás

On Fri, Jan 24, 2025 at 09:23:47AM +0000, Barnabás Pőcze wrote:
> Hi
>
>
> 2025. január 22., szerda 10:03 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:
>
> > Hi Barnabás
> >
> > On Tue, Jan 14, 2025 at 06:21:56PM +0000, Barnabás Pőcze wrote:
> > > The compiler generated functions are not appropriate, so
> > > delete the copy/move constructor/assignment to avoid
> > > potential issues.
> > >
> > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> >
> > The class has this member
> > 	std::list<std::unique_ptr<Event>> events_;
> >
> > Is there a copy constructor generated for the class even if
> > unique_ptr<> do not provide one ?
>
> That list would indeed cause the copy constructor/assignment to be deleted,
> however, move constructor/assignment would still be generated, which would
> be incorrect since the owning `event_base` pointer is stored in a raw pointer.
> However, the class has an `std::mutex` member, so there are in fact no copy/move
> constructors/assignment operators defined by default.
>
> So this change does not really change much, my bad, this can be probably be ignored.
> Nonetheless, I have run into at least one situation in libcamera where a move
> constructor was defined by the compiler but semantically it was wrong. Specifically,
> it was `EventNotifier`; I am wondering if `Object` should disable copy/move altogether.
>
> In any case, I'd support it if libcamera would enforce the *rule of 0/5* more strictly.
> ( https://en.cppreference.com/w/cpp/language/rule_of_three )

Are you aware of any tooling that could help us detecting which
classes would need attention ?

>
>
> Regards,
> Barnabás Pőcze
>
> >
> > > ---
> > >  src/apps/common/event_loop.h | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/src/apps/common/event_loop.h b/src/apps/common/event_loop.h
> > > index d7d012c76..4e8dd0a46 100644
> > > --- a/src/apps/common/event_loop.h
> > > +++ b/src/apps/common/event_loop.h
> > > @@ -13,6 +13,8 @@
> > >  #include <memory>
> > >  #include <mutex>
> > >
> > > +#include <libcamera/base/class.h>
> > > +
> > >  #include <event2/util.h>
> > >
> > >  struct event_base;
> > > @@ -43,8 +45,11 @@ public:
> > >  			   std::function<void()> &&handler);
> > >
> > >  private:
> > > +	LIBCAMERA_DISABLE_COPY_AND_MOVE(EventLoop)
> > > +
> > >  	struct Event {
> > >  		Event(std::function<void()> &&callback);
> > > +		LIBCAMERA_DISABLE_COPY_AND_MOVE(Event)
> > >  		~Event();
> > >
> > >  		static void dispatch(int fd, short events, void *arg);
> > > --
> > > 2.48.0
> > >
> > >
> >
Barnabás Pőcze Jan. 24, 2025, 11:25 a.m. UTC | #4
2025. január 24., péntek 12:17 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:

> Hi Barnabás
> 
> On Fri, Jan 24, 2025 at 09:23:47AM +0000, Barnabás Pőcze wrote:
> > Hi
> >
> >
> > 2025. január 22., szerda 10:03 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:
> >
> > > Hi Barnabás
> > >
> > > On Tue, Jan 14, 2025 at 06:21:56PM +0000, Barnabás Pőcze wrote:
> > > > The compiler generated functions are not appropriate, so
> > > > delete the copy/move constructor/assignment to avoid
> > > > potential issues.
> > > >
> > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > >
> > > The class has this member
> > > 	std::list<std::unique_ptr<Event>> events_;
> > >
> > > Is there a copy constructor generated for the class even if
> > > unique_ptr<> do not provide one ?
> >
> > That list would indeed cause the copy constructor/assignment to be deleted,
> > however, move constructor/assignment would still be generated, which would
> > be incorrect since the owning `event_base` pointer is stored in a raw pointer.
> > However, the class has an `std::mutex` member, so there are in fact no copy/move
> > constructors/assignment operators defined by default.
> >
> > So this change does not really change much, my bad, this can be probably be ignored.
> > Nonetheless, I have run into at least one situation in libcamera where a move
> > constructor was defined by the compiler but semantically it was wrong. Specifically,
> > it was `EventNotifier`; I am wondering if `Object` should disable copy/move altogether.
> >
> > In any case, I'd support it if libcamera would enforce the *rule of 0/5* more strictly.
> > ( https://en.cppreference.com/w/cpp/language/rule_of_three )
> 
> Are you aware of any tooling that could help us detecting which
> classes would need attention ?

clang-tidy for example: https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/special-member-functions.html


Regards,
Barnabás Pőcze

> 
> >
> >
> > Regards,
> > Barnabás Pőcze
> >
> > >
> > > > ---
> > > >  src/apps/common/event_loop.h | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/src/apps/common/event_loop.h b/src/apps/common/event_loop.h
> > > > index d7d012c76..4e8dd0a46 100644
> > > > --- a/src/apps/common/event_loop.h
> > > > +++ b/src/apps/common/event_loop.h
> > > > @@ -13,6 +13,8 @@
> > > >  #include <memory>
> > > >  #include <mutex>
> > > >
> > > > +#include <libcamera/base/class.h>
> > > > +
> > > >  #include <event2/util.h>
> > > >
> > > >  struct event_base;
> > > > @@ -43,8 +45,11 @@ public:
> > > >  			   std::function<void()> &&handler);
> > > >
> > > >  private:
> > > > +	LIBCAMERA_DISABLE_COPY_AND_MOVE(EventLoop)
> > > > +
> > > >  	struct Event {
> > > >  		Event(std::function<void()> &&callback);
> > > > +		LIBCAMERA_DISABLE_COPY_AND_MOVE(Event)
> > > >  		~Event();
> > > >
> > > >  		static void dispatch(int fd, short events, void *arg);
> > > > --
> > > > 2.48.0
> > > >
> > > >
> > >
>

Patch
diff mbox series

diff --git a/src/apps/common/event_loop.h b/src/apps/common/event_loop.h
index d7d012c76..4e8dd0a46 100644
--- a/src/apps/common/event_loop.h
+++ b/src/apps/common/event_loop.h
@@ -13,6 +13,8 @@ 
 #include <memory>
 #include <mutex>
 
+#include <libcamera/base/class.h>
+
 #include <event2/util.h>
 
 struct event_base;
@@ -43,8 +45,11 @@  public:
 			   std::function<void()> &&handler);
 
 private:
+	LIBCAMERA_DISABLE_COPY_AND_MOVE(EventLoop)
+
 	struct Event {
 		Event(std::function<void()> &&callback);
+		LIBCAMERA_DISABLE_COPY_AND_MOVE(Event)
 		~Event();
 
 		static void dispatch(int fd, short events, void *arg);