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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

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.

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

Bart