Re: [libgpiod][PATCH] treewide: rework struct gpiod_line_bulk

[Kent Gibson <warthog618@xxxxxxxxx>]

On Fri, Oct 23, 2020 at 11:28:31AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> 
> Current implementation of struct gpiod_line_bulk uses stack memory
> excessively. The structure is big: it's an array 64 pointers + 4 bytes
> size. That amounts to 260 bytes on 32-bit and 516 bytes on 64-bit
> architectures respectively. It's also used everywhere as all functions
> dealing with single lines eventually end up calling bulk counterparts.
> 
> The rework addresses it by making the bulk structure opaque and
> providing appropriate interfaces for library users while retaining a way
> for internal users to allocate single line bulks on the stack.
> 
> The macro-based loop has been removed. In its place we provide a function
> iterating over all lines held by a bulk and calling the provided callback
> function for each line as well as a new line bulk iterator which works
> similarily to chip and line iterators.
> 
> Since bulk operations can now fail, a bunch of test-cases has been added
> to cover the relevant code.
> 
> While at it: using the word offset both when referring to line's HW
> offset in a chip as well as the offset in a bulk leads to confusion.
> This patch renames the bulk offset to index.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> ---
>  bindings/cxx/gpiod.hpp        |  18 ++-
>  bindings/cxx/line_bulk.cpp    | 112 +++++++++--------
>  bindings/python/gpiodmodule.c | 182 +++++++++++++++++++--------
>  include/gpiod.h               | 169 +++++++++++++------------
>  lib/core.c                    | 178 +++++++++++++++++++--------
>  lib/ctxless.c                 |  53 +++++---
>  lib/helpers.c                 |  61 +++++----
>  lib/iter.c                    |  33 +++++
>  tests/Makefile.am             |   1 +
>  tests/gpiod-test.h            |   5 +
>  tests/tests-bulk.c            | 158 ++++++++++++++++++++++++
>  tests/tests-chip.c            |  59 +++++----
>  tests/tests-event.c           |  73 +++++++----
>  tests/tests-iter.c            |  26 ++++
>  tests/tests-line.c            | 225 +++++++++++++---------------------
>  15 files changed, 888 insertions(+), 465 deletions(-)
>  create mode 100644 tests/tests-bulk.c
> 
> diff --git a/bindings/cxx/gpiod.hpp b/bindings/cxx/gpiod.hpp
> index 8dfc172..83f543f 100644
> --- a/bindings/cxx/gpiod.hpp
> +++ b/bindings/cxx/gpiod.hpp
> @@ -626,18 +626,18 @@ public:
>  
>  	/**
>  	 * @brief Get the line at given offset.
> -	 * @param offset Offset of the line to get.
> +	 * @param index Offset of the line to get.
>  	 * @return Reference to the line object.
>  	 */

The comment still says "Offset" and so is still confusing.
Change it to something like: "Index of the line within the bulk"?

And document what happens if index >= size()?

Similarly elsewhere.

That is just the one thing that got my attention - I'll try to
find time to go over the whole patch in the next few days.

Cheers,
Kent.