[PATCH v2 1/3] libcamera: framebuffer_allocator: Move from argument in constructor

[Barnabás Pőcze]

Hi


2024. március 11., hétfő 18:01 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:

> Hi Barnabás
> 
> On Sun, Mar 10, 2024 at 02:30:33PM +0000, Barnabás Pőcze wrote:
> > The single argument, of type `std::shared_ptr<Camera>`,
> > is passed by value, so it can simply be moved from in order to
> > avoid calling the copy constructor.
> >
> > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > ---
> >  src/libcamera/framebuffer_allocator.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> > index 94389735..8cf45ab2 100644
> > --- a/src/libcamera/framebuffer_allocator.cpp
> > +++ b/src/libcamera/framebuffer_allocator.cpp
> > @@ -59,7 +59,7 @@ LOG_DEFINE_CATEGORY(Allocator)
> >   * \param[in] camera The camera
> >   */
> >  FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr<Camera> camera)
> > -	: camera_(camera)
> > +	: camera_(std::move(camera))
> 
> However, if the caller of the FrameBufferAllocator constructor uses
> move() when calling the constructur, this will not increase the
> shared_ptr<> reference count and this could lead to a path where the
> object held by the shared_ptr<> (the camera in this case) is released
> before the FrameBufferAllocator instance referencing it ?

That is not possible. The constructor's argument holds a reference to the camera
object preventing it from being destroyed.


> 
> Do yuo think it's an issue ? Applications shouldn't
> 
>         FrameBufferAllocator(std::move(camera))
> 
> but is there anything preventing them from doing so ?

The observable behaviour for the caller does not really change after the proposed change.
If someone uses `FrameBufferAllocator(std::move(camera))`, then that will construct
a temporary with the shared_ptr's move constructor, so `camera` will point to nullptr
afterwards, regardless of what is done inside the constructor. This is what happens
now and what will happen after this change.

Previously if someone did

  FrameBufferAllocator { ... }

then there would be
  * two copy constructor invocations, or
  * one move constructor and one copy constructor invocation.
depending on the type of `...` above.

The first one when constructing the temporary for the argument,
and the second one when constructing the member variable from the argument.

With the proposed change, there will be at most one copy constructor call
when constructing a FrameBufferAllocator.


> 
> >  {
> >  }
> >
> > --
> > 2.44.0
> >
> >


Regards,
Barnabás Pőcze