[PATCH] libcamera: Add static Object::Deleter function

[Barnabás Pőcze]

Hi


2024. október 24., csütörtök 11:11 keltezéssel, Cheng-Hao Yang <chenghaoyang at chromium.org> írta:

> Hi Barnabás,
> 
> On Thu, Oct 24, 2024 at 1:33 AM Barnabás Pőcze <pobrn at protonmail.com> wrote:
> >
> > Hi
> >
> >
> > 2024. október 23., szerda 18:58 keltezéssel, Harvey Yang <chenghaoyang at 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 at 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
> > >