Re: [libgpiod][RFC PATCH] bindings: cxx: demote the line's parent chip reference to a weak_ptr

[Helmut Grohne <helmut.grohne@xxxxxxxxxx>]

On Mon, Oct 19, 2020 at 02:17:49PM +0200, Bartosz Golaszewski wrote:
> On Fri, Oct 16, 2020 at 3:38 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> >
> > On Fri, Oct 16, 2020 at 12:29 PM Helmut Grohne <helmut.grohne@xxxxxxxxxx> wrote:
> > >
> >
> > [snip]
> >
> > > > +chip::chip(const ::std::weak_ptr<::gpiod_chip>& chip_ptr)
> > > > +     : _m_chip(chip_ptr)
> > > > +{
> > > > +
> > > > +}
> > > > +
> > >
> > > I think what happens here is that you upgrade a weak_ptr to a
> > > shared_ptr. Wouldn't it be more natural to request a
> > >
> > >     ::std::shared_ptr<::gpiod_chip> &&
> > >
> > > here and thus make the ownership-taking more explicit? It would be done
> > > on the caller-side and thus be more transparent. Stuffing weak_ptrs
> > > should continue to work.
> > >
> >
> > Sure, sounds good.
> >
> 
> After a second look - I'm not sure if this is actually better. By
> taking weak_ptr reference as argument we benefit from implicit
> conversion to shared_ptr via shared_ptr's constructor taking weak_ptr
> as argument. What you propose would require us to always instantiate a
> shared_ptr in the argument list when calling the chip's constructor
> and makes code uglier in the end IMO.

On a second look, the use of an rvalue reference is suboptimal indeed.
The idea behind my change was this: Since chip stores a shared_ptr, it
can as well consume one. Instead of what I proposed, it should simply
take it by value (not rvalue):

    ::std::shared_ptr<::gpiod_chip>

An existing shared_ptr can be moved into the constructor and then moved
into the member variable. Doing so allows passing a shared_ptr around
without touching reference counts (which are prone to cache line
bouncing). When passing it by value, the implicit conversion from
weak_ptr should work again. Thus the caller would increase the reference
count and the chip would merely gain ownership of the shared_ptr and
move it around.

For reference, see Scott Myers' Effective Modern C++ "Item 41: Consider
pass by value for copyable parameters that are cheap to move and always
copied."

So yeah, it doesn't work the way I wrote initially, because I added the
rvalue reference.

Helmut