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

[Jacopo Mondi]

Hi Barnabás

On Mon, Mar 11, 2024 at 05:42:55PM +0000, Barnabás Pőcze wrote:
> 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.

I think the key point is also that std::move() of a shared_ptr<>
effectively resets it, so it's not something anyone would do, which
makes my argument moot

>
> 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.
>

For reference, this is the code generated by goldbot for the two use
cases with the following code snippet:

-------------------------------------------------------------------------------
#include <memory>

using namespace std;

class klass
{
public:
    klass(shared_ptr<int> i)
        : i_(i)
        // : i_(move(i))
    {
    }

private:
    shared_ptr<int> i_;
};

int main()
{
    shared_ptr<int> i = std::make_shared<int>(5);

    klass k(i);

    return 0;
}
-------------------------------------------------------------------------------


* move)
	mov     rbx, QWORD PTR [rbp-24]
        mov     rax, QWORD PTR [rbp-32]
        mov     rdi, rax
        call    std::remove_reference<std::shared_ptr<int>&>::type&& std::move<std::shared_ptr<int>&>(std::shared_ptr<int>&)
        mov     rsi, rax
        mov     rdi, rbx
        call    std::shared_ptr<int>::shared_ptr(std::shared_ptr<int>&&) [complete object constructor]


* non move)
        mov     rax, QWORD PTR [rbp-8]
        mov     rdx, QWORD PTR [rbp-16]
        mov     rsi, rdx
        mov     rdi, rax
        call    std::shared_ptr<int>::shared_ptr(std::shared_ptr<int> const&) [complete object constructor]

I hoped for the compiler to be smart enough to avoid calling the copy
constructor for both the temporary object and the class member
initialization, but that doesn't seem the case, so

Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

>
> >
> > >  {
> > >  }
> > >
> > > --
> > > 2.44.0
> > >
> > >
>
>
> Regards,
> Barnabás Pőcze