Re: [libgpiod v2][PATCH v6 5/5] bindings: cxx: add implementation

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Mon, May 2, 2022 at 3:54 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Mon, May 02, 2022 at 02:34:34PM +0200, Bartosz Golaszewski wrote:
> > On Wed, Apr 27, 2022 at 8:02 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > >
> > > On Tue, Apr 26, 2022 at 02:50:23PM +0200, Bartosz Golaszewski wrote:
> > > > This contains the actual implementation of the v2 C++ bindings.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <brgl@xxxxxxxx>
> > >
> > > [snip]
> > >
> > > > +
> > > > +GPIOD_CXX_API ::std::size_t line_request::num_lines(void) const
> > > > +{
> > > > +     this->_m_priv->throw_if_released();
> > > > +
> > > > +     return ::gpiod_line_request_get_num_lines(this->_m_priv->request.get());
> > > > +}
> > > > +
> > > > +GPIOD_CXX_API line::offsets line_request::offsets(void) const
> > > > +{
> > > > +     this->_m_priv->throw_if_released();
> > > > +
> > > > +     ::std::vector<unsigned int> buf(this->num_lines());
> > > > +     line::offsets offsets(this->num_lines());
> > > > +
> > > > +     ::gpiod_line_request_get_offsets(this->_m_priv->request.get(), buf.data());
> > > > +
> > > > +     auto num_lines = this->num_lines();
> > > > +     for (unsigned int i = 0; i < num_lines; i++)
> > > > +             offsets[i] = buf[i];
> > > > +
> > > > +     return offsets;
> > > > +}
> > > > +
> > >
> > > My previous comment was "Cache num_lines locally rather than calling
> > > num_lines() several times."
> > >
> > > You cached it in the wrong place - it should be first thing in the
> > > function, so you only call it once, not three times.
> > >
> >
> > I fixed it when applying.
> >
> > > And the throw_if_released() is still "redundant as this->num_lines()
> > > also does it", and it is the first thing called here - after the
> > > throw_if_released().
> > >
> >
> > I think it's still worth having it here because at least the code
> > makes it clear, we need to have a valid request here. It's not like
> > it's a hot path where this additional function call would matter IMO.
> >
>
> So add a comment?
> Pointless work is pointless work, hot path or not.
>
> > > And I would've made this patch 3/5, not 5/5.
> > >
> > > But I'm fine with this set going in either way - in fact give its size
> > > I'd rather see minor tweaks applied later than go through another round
> > > of review.
> > >
> > > For the series:
> > >
> > > Reviewed-by: Kent Gibson <warthog618@xxxxxxxxx>
> >
> > Thanks I applied it for now as it is, let's get it into master with
> > the Python bindings and then polish it some more there.
> >
> > >
> > > I really would also like to see some feedback from C++ developers that
> > > will actually be using it, as they have a bigger stake in it than I do.
> > > But that is up to them.
> > >
> >
> > Indeed, any idea where to post that to get some free code reviews from
> > C++ devs? :)
> >
>
> That does raise the issue of whether libgpiod should have a forum other
> than this mailing list.
>
> I was hoping to at least nudge the others on the CC list ¯\_(ツ)_/¯.
> If no one is sufficiently interested to bother reviewing, or even acking,
> the C++ bindings, perhaps deprecate them instead ;-)?
>

Well, I've received enough emails over the years with questions about
C++ bindings to assume they are being used (although Python bindings
seem to be preferred indeed). I think it's just a significant amount
of work to go through 10000 LOC just for fun, hence the lack of
reviews. :)

Anyway, I'm pretty happy with how they turned out and I intend to keep
them. Hopefully nothing is too broken to require an incompatible fix.
I'll try to find some reviewers outside the list in the meantime.

Bart