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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Mon, May 02, 2022 at 07:41:56PM +0200, Bartosz Golaszewski wrote:
> 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. :)

That was why I asked to have the original patch split.
An end user/lite review could just cover the headers and examples, and
optionally the tools as additional examples, and say "yeah that makes
sense", or "why the hell did you do it that way?" and not have to concern
themselves with the impl or tests.

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

Btw, by "deprecate" I didn't mean throw them away, just delay merging
them into master until you get some user feedback.
And even then it was in jest.  Mostly.

Cheers,
Kent.