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

[Kent Gibson <warthog618@xxxxxxxxx>]

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 ;-)?

Cheers,
Kent.