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

[Bartosz Golaszewski <bgolaszewski@baylibre.com>]

On Mon, Oct 19, 2020 at 2:38 PM Helmut Grohne <helmut.grohne@intenta.de> wrote:
>
> 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@bgdev.pl> wrote:
> > >
> > > On Fri, Oct 16, 2020 at 12:29 PM Helmut Grohne <helmut.grohne@intenta.de> 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

But this still forces us to do

    return chip(::std::shared_ptr<::gpiod_chip>(this->_m_owner));

instead of a much more elegant

    return chip(this->_m_owner);

in line.cpp and there's an even less elegant thing in iter.cpp. Or am
I missing something?

Bartosz