Message ID | 20241023165812.868221-1-chenghaoyang@chromium.org |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
Hi 2024. október 23., szerda 18:58 keltezéssel, Harvey Yang <chenghaoyang@chromium.org> írta: > This patch allows a smart pointer of Object to be destructed in other > threads. > > There will be multiple usages of smart pointers of Object. This patch > adds the deleter function to avoid duplicated code. > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > --- > include/libcamera/base/object.h | 2 ++ > src/libcamera/base/object.cpp | 11 +++++++++++ > 2 files changed, 13 insertions(+) > > diff --git a/include/libcamera/base/object.h b/include/libcamera/base/object.h > index 508773cd0..c4522d480 100644 > --- a/include/libcamera/base/object.h > +++ b/include/libcamera/base/object.h > @@ -24,6 +24,8 @@ class Thread; > class Object > { > public: > + static void Deleter(Object *obj); I think a functor would be preferable, see camera.cpp:Camera::create(): struct Deleter { void operator()(Object *obj) const { if (Thread::current() == obj->thread()) delete obj; else obj->deleteLater(); } }; Or is there a reason why this wouldn't work? Regards, Barnabás Pőcze > + > Object(Object *parent = nullptr); > virtual ~Object(); > > diff --git a/src/libcamera/base/object.cpp b/src/libcamera/base/object.cpp > index 745d2565a..2c04b99a5 100644 > --- a/src/libcamera/base/object.cpp > +++ b/src/libcamera/base/object.cpp > @@ -59,6 +59,17 @@ LOG_DEFINE_CATEGORY(Object) > * \sa Message, Signal, Thread > */ > > +/** > + * \brief A deleter function that calls Object::deleteLater > + * \param[in] obj The object itself > + * > + * The static deleter function that's used in smart pointers. > + */ > +void Object::Deleter(Object *obj) > +{ > + obj->deleteLater(); > +} > + > /** > * \brief Construct an Object instance > * \param[in] parent The object parent > -- > 2.47.0.105.g07ac214952-goog >
Hi Barnabás, On Thu, Oct 24, 2024 at 1:33 AM Barnabás Pőcze <pobrn@protonmail.com> wrote: > > Hi > > > 2024. október 23., szerda 18:58 keltezéssel, Harvey Yang <chenghaoyang@chromium.org> írta: > > > This patch allows a smart pointer of Object to be destructed in other > > threads. > > > > There will be multiple usages of smart pointers of Object. This patch > > adds the deleter function to avoid duplicated code. > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > --- > > include/libcamera/base/object.h | 2 ++ > > src/libcamera/base/object.cpp | 11 +++++++++++ > > 2 files changed, 13 insertions(+) > > > > diff --git a/include/libcamera/base/object.h b/include/libcamera/base/object.h > > index 508773cd0..c4522d480 100644 > > --- a/include/libcamera/base/object.h > > +++ b/include/libcamera/base/object.h > > @@ -24,6 +24,8 @@ class Thread; > > class Object > > { > > public: > > + static void Deleter(Object *obj); > > I think a functor would be preferable, see camera.cpp:Camera::create(): > > struct Deleter { > void operator()(Object *obj) const > { > if (Thread::current() == obj->thread()) > delete obj; > else > obj->deleteLater(); > } > }; > > Or is there a reason why this wouldn't work? True, thanks for the notice, and it works. I also just found that it's stated in object.cpp: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/base/object.cpp#n143 Do you think I should add this functor to avoid duplication, or the developers should copy and paste the above functor each time for a derived class? BR, Harvey > > > Regards, > Barnabás Pőcze > > > + > > Object(Object *parent = nullptr); > > virtual ~Object(); > > > > diff --git a/src/libcamera/base/object.cpp b/src/libcamera/base/object.cpp > > index 745d2565a..2c04b99a5 100644 > > --- a/src/libcamera/base/object.cpp > > +++ b/src/libcamera/base/object.cpp > > @@ -59,6 +59,17 @@ LOG_DEFINE_CATEGORY(Object) > > * \sa Message, Signal, Thread > > */ > > > > +/** > > + * \brief A deleter function that calls Object::deleteLater > > + * \param[in] obj The object itself > > + * > > + * The static deleter function that's used in smart pointers. > > + */ > > +void Object::Deleter(Object *obj) > > +{ > > + obj->deleteLater(); > > +} > > + > > /** > > * \brief Construct an Object instance > > * \param[in] parent The object parent > > -- > > 2.47.0.105.g07ac214952-goog > >
Hi 2024. október 24., csütörtök 11:11 keltezéssel, Cheng-Hao Yang <chenghaoyang@chromium.org> írta: > Hi Barnabás, > > On Thu, Oct 24, 2024 at 1:33 AM Barnabás Pőcze <pobrn@protonmail.com> wrote: > > > > Hi > > > > > > 2024. október 23., szerda 18:58 keltezéssel, Harvey Yang <chenghaoyang@chromium.org> írta: > > > > > This patch allows a smart pointer of Object to be destructed in other > > > threads. > > > > > > There will be multiple usages of smart pointers of Object. This patch > > > adds the deleter function to avoid duplicated code. > > > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > --- > > > include/libcamera/base/object.h | 2 ++ > > > src/libcamera/base/object.cpp | 11 +++++++++++ > > > 2 files changed, 13 insertions(+) > > > > > > diff --git a/include/libcamera/base/object.h b/include/libcamera/base/object.h > > > index 508773cd0..c4522d480 100644 > > > --- a/include/libcamera/base/object.h > > > +++ b/include/libcamera/base/object.h > > > @@ -24,6 +24,8 @@ class Thread; > > > class Object > > > { > > > public: > > > + static void Deleter(Object *obj); > > > > I think a functor would be preferable, see camera.cpp:Camera::create(): > > > > struct Deleter { > > void operator()(Object *obj) const > > { > > if (Thread::current() == obj->thread()) > > delete obj; > > else > > obj->deleteLater(); > > } > > }; > > > > Or is there a reason why this wouldn't work? > > True, thanks for the notice, and it works. > > I also just found that it's stated in object.cpp: > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/base/object.cpp#n143 Ahh you're right, I didn't notice that. > > Do you think I should add this functor to avoid duplication, > or the developers should copy and paste the above > functor each time for a derived class? Well, I think having just the functor here is a bit suboptimal because generally it makes sense to keep the deleter and the smart pointer construction close to each other. So now I am thinking about something like this: template<typename T, typename... Args> static std::unique_ptr<T, Deleter> create(Args&&... args) { return std::unique_ptr<T, Deleter>(new T(std::forward<Args>(args)...)); } And if you want a shared_ptr, you can simply do std::shared_ptr<T>(Object::create<T>(...)); Or I guess you could add two functions: `make_unique` and `make_shared` instead of `create`. I don't know what your use cases are, but I believe this would likely allow you to make simplifications. Regards, Barnabás Pőcze > > BR, > Harvey > > > > > > > Regards, > > Barnabás Pőcze > > > > > + > > > Object(Object *parent = nullptr); > > > virtual ~Object(); > > > > > > diff --git a/src/libcamera/base/object.cpp b/src/libcamera/base/object.cpp > > > index 745d2565a..2c04b99a5 100644 > > > --- a/src/libcamera/base/object.cpp > > > +++ b/src/libcamera/base/object.cpp > > > @@ -59,6 +59,17 @@ LOG_DEFINE_CATEGORY(Object) > > > * \sa Message, Signal, Thread > > > */ > > > > > > +/** > > > + * \brief A deleter function that calls Object::deleteLater > > > + * \param[in] obj The object itself > > > + * > > > + * The static deleter function that's used in smart pointers. > > > + */ > > > +void Object::Deleter(Object *obj) > > > +{ > > > + obj->deleteLater(); > > > +} > > > + > > > /** > > > * \brief Construct an Object instance > > > * \param[in] parent The object parent > > > -- > > > 2.47.0.105.g07ac214952-goog > > >
Hi Barnabás, On Thu, Oct 24, 2024 at 8:59 PM Barnabás Pőcze <pobrn@protonmail.com> wrote: > > Hi > > > 2024. október 24., csütörtök 11:11 keltezéssel, Cheng-Hao Yang <chenghaoyang@chromium.org> írta: > > > Hi Barnabás, > > > > On Thu, Oct 24, 2024 at 1:33 AM Barnabás Pőcze <pobrn@protonmail.com> wrote: > > > > > > Hi > > > > > > > > > 2024. október 23., szerda 18:58 keltezéssel, Harvey Yang <chenghaoyang@chromium.org> írta: > > > > > > > This patch allows a smart pointer of Object to be destructed in other > > > > threads. > > > > > > > > There will be multiple usages of smart pointers of Object. This patch > > > > adds the deleter function to avoid duplicated code. > > > > > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > > --- > > > > include/libcamera/base/object.h | 2 ++ > > > > src/libcamera/base/object.cpp | 11 +++++++++++ > > > > 2 files changed, 13 insertions(+) > > > > > > > > diff --git a/include/libcamera/base/object.h b/include/libcamera/base/object.h > > > > index 508773cd0..c4522d480 100644 > > > > --- a/include/libcamera/base/object.h > > > > +++ b/include/libcamera/base/object.h > > > > @@ -24,6 +24,8 @@ class Thread; > > > > class Object > > > > { > > > > public: > > > > + static void Deleter(Object *obj); > > > > > > I think a functor would be preferable, see camera.cpp:Camera::create(): > > > > > > struct Deleter { > > > void operator()(Object *obj) const > > > { > > > if (Thread::current() == obj->thread()) > > > delete obj; > > > else > > > obj->deleteLater(); > > > } > > > }; > > > > > > Or is there a reason why this wouldn't work? > > > > True, thanks for the notice, and it works. > > > > I also just found that it's stated in object.cpp: > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/base/object.cpp#n143 > > Ahh you're right, I didn't notice that. > > > > > > Do you think I should add this functor to avoid duplication, > > or the developers should copy and paste the above > > functor each time for a derived class? > > Well, I think having just the functor here is a bit suboptimal because generally > it makes sense to keep the deleter and the smart pointer construction close > to each other. > > So now I am thinking about something like this: > > template<typename T, typename... Args> > static std::unique_ptr<T, Deleter> create(Args&&... args) > { > return std::unique_ptr<T, Deleter>(new T(std::forward<Args>(args)...)); > } > > And if you want a shared_ptr, you can simply do > > std::shared_ptr<T>(Object::create<T>(...)); I encountered some undefined symbols with this template function, which I normally fixed by putting the definition in the header file instead of the source file. However, it seems that including `base/thread.h` in `base/object.h` causes other issues (cyclic dependencies...?). > > Or I guess you could add two functions: `make_unique` and `make_shared` instead of `create`. I think defining make_unique of Object would overwrite the default one, and thus cause some issues in other Object derived classes' usages. Let's avoid this as well. > > I don't know what your use cases are, but I believe this would likely allow > you to make simplifications. My use case currently is for std::unique_ptr [1], FYI. As there doesn't seem to be a clean way to do this, we can also just drop this patch. WDYT? BR, Harvey [1]:https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/ipa/mtkisp7/mtkisp7.cpp;l=43 > > > Regards, > Barnabás Pőcze > > > > > BR, > > Harvey > > > > > > > > > > > Regards, > > > Barnabás Pőcze > > > > > > > + > > > > Object(Object *parent = nullptr); > > > > virtual ~Object(); > > > > > > > > diff --git a/src/libcamera/base/object.cpp b/src/libcamera/base/object.cpp > > > > index 745d2565a..2c04b99a5 100644 > > > > --- a/src/libcamera/base/object.cpp > > > > +++ b/src/libcamera/base/object.cpp > > > > @@ -59,6 +59,17 @@ LOG_DEFINE_CATEGORY(Object) > > > > * \sa Message, Signal, Thread > > > > */ > > > > > > > > +/** > > > > + * \brief A deleter function that calls Object::deleteLater > > > > + * \param[in] obj The object itself > > > > + * > > > > + * The static deleter function that's used in smart pointers. > > > > + */ > > > > +void Object::Deleter(Object *obj) > > > > +{ > > > > + obj->deleteLater(); > > > > +} > > > > + > > > > /** > > > > * \brief Construct an Object instance > > > > * \param[in] parent The object parent > > > > -- > > > > 2.47.0.105.g07ac214952-goog > > > >
diff --git a/include/libcamera/base/object.h b/include/libcamera/base/object.h index 508773cd0..c4522d480 100644 --- a/include/libcamera/base/object.h +++ b/include/libcamera/base/object.h @@ -24,6 +24,8 @@ class Thread; class Object { public: + static void Deleter(Object *obj); + Object(Object *parent = nullptr); virtual ~Object(); diff --git a/src/libcamera/base/object.cpp b/src/libcamera/base/object.cpp index 745d2565a..2c04b99a5 100644 --- a/src/libcamera/base/object.cpp +++ b/src/libcamera/base/object.cpp @@ -59,6 +59,17 @@ LOG_DEFINE_CATEGORY(Object) * \sa Message, Signal, Thread */ +/** + * \brief A deleter function that calls Object::deleteLater + * \param[in] obj The object itself + * + * The static deleter function that's used in smart pointers. + */ +void Object::Deleter(Object *obj) +{ + obj->deleteLater(); +} + /** * \brief Construct an Object instance * \param[in] parent The object parent
This patch allows a smart pointer of Object to be destructed in other threads. There will be multiple usages of smart pointers of Object. This patch adds the deleter function to avoid duplicated code. Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> --- include/libcamera/base/object.h | 2 ++ src/libcamera/base/object.cpp | 11 +++++++++++ 2 files changed, 13 insertions(+)