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

[Kent Gibson <warthog618@xxxxxxxxx>]

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.

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

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>

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.

Cheers,
Kent.