Message ID | 20250114182143.1773762-3-pobrn@protonmail.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
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 > >
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 > > > > >
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 > > > > > > > >
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 > > > > > > > > > > > >
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);
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(+)