[libcamera-devel] [PATCH/RFC 2/3] libcamera: utils: enumerate: Use named fields for result

[Niklas Söderlund]

Hi Laurent,

Thanks for your work.

On 2021-04-23 05:09:31 +0300, Laurent Pinchart wrote:
> Returning a pair of { index, value } from container enumeration is
> error-prone, as the index and value can easily be swapped. Use a
> structure with named index and value fields instead.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

I really liked the API in 1/3 and my preference would be to keep that.  
For that reason I do not provide my tag even if I find the patch itself 
to be good.

> ---
>  include/libcamera/internal/utils.h | 16 +++++++++++-----
>  src/libcamera/utils.cpp            | 13 +++++++------
>  test/utils.cpp                     | 24 ++++++++++++------------
>  3 files changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h
> index 9372a75889ac..54a6b6d283fb 100644
> --- a/include/libcamera/internal/utils.h
> +++ b/include/libcamera/internal/utils.h
> @@ -16,7 +16,6 @@
>  #include <string>
>  #include <string.h>
>  #include <sys/time.h>
> -#include <utility>
>  #include <vector>
>  
>  #ifndef __DOXYGEN__
> @@ -237,12 +236,19 @@ namespace details {
>  template<typename Base>
>  class enumerate_iterator
>  {
> -private:
> -	using base_reference = typename std::iterator_traits<Base>::reference;
> -
>  public:
>  	using difference_type = typename std::iterator_traits<Base>::difference_type;
> -	using value_type = std::pair<const difference_type, base_reference>;
> +
> +private:
> +	using base_reference = typename std::iterator_traits<Base>::reference;
> +
> +	struct result {
> +		const difference_type index;
> +		base_reference value;
> +	};
> +
> +public:
> +	using value_type = result;
>  	using pointer = value_type *;
>  	using reference = value_type &;
>  	using iterator_category = typename std::iterator_traits<Base>::iterator_category;
> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> index ff9a5832b10e..b4b4180c1337 100644
> --- a/src/libcamera/utils.cpp
> +++ b/src/libcamera/utils.cpp
> @@ -481,20 +481,21 @@ std::string libcameraSourcePath()
>   * in their ability to replace for loops that require access to a loop counter.
>   * The enumerate() function solves this problem by wrapping the \a iterable in
>   * an adapter that, when used as a range-expression, will provide iterators
> - * whose value_type is a pair of index and value reference.
> + * whose value_type exposes both the element's value and its index.
>   *
>   * The iterable must support std::begin() and std::end(). This includes all
>   * containers provided by the standard C++ library, as well as C-style arrays.
>   *
> - * A typical usage pattern would use structured binding to store the index and
> - * value in two separate variables:
> + * The iterator's value_type is a structure that aggregates the index and value
> + * reference in two named members. The index is always const, and the value
> + * reference is conditionally const depending on the iterable. A typical usage
> + * pattern would be:
>   *
>   * \code{.cpp}
>   * std::vector<int> values = ...;
>   *
> - * for (auto [index, value] : utils::enumerate(values)) {
> - * 	...
> - * }
> + * for (const auto v : utils::enumerate(values))
> + * 	std::cout << "- index " << v.index << ", value " << v.value << std::endl;
>   * \endcode
>   *
>   * \return A value of unspecified type that, when used in a range-based for
> diff --git a/test/utils.cpp b/test/utils.cpp
> index 7e24c71e4775..06ce5301a74e 100644
> --- a/test/utils.cpp
> +++ b/test/utils.cpp
> @@ -79,16 +79,16 @@ protected:
>  		std::vector<int> integers{ 1, 2, 3, 4, 5 };
>  		int i = 0;
>  
> -		for (auto [index, value] : utils::enumerate(integers)) {
> -			if (index != i || value != i + 1) {
> +		for (const auto v : utils::enumerate(integers)) {
> +			if (v.index != i || v.value != i + 1) {
>  				cerr << "utils::enumerate(<vector>) test failed: i=" << i
> -				     << ", index=" << index << ", value=" << value
> -				     << std::endl;
> +				     << ", index=" << v.index << ", value=" << v.value
> +				     << endl;
>  				return TestFail;
>  			}
>  
>  			/* Verify that we can modify the value. */
> -			--value;
> +			--v.value;
>  			++i;
>  		}
>  
> @@ -100,10 +100,10 @@ protected:
>  		Span<const int> span{ integers };
>  		i = 0;
>  
> -		for (auto [index, value] : utils::enumerate(span)) {
> -			if (index != i || value != i) {
> +		for (const auto v : utils::enumerate(span)) {
> +			if (v.index != i || v.value != i) {
>  				cerr << "utils::enumerate(<span>) test failed: i=" << i
> -				     << ", index=" << index << ", value=" << value
> +				     << ", index=" << v.index << ", value=" << v.value
>  				     << std::endl;
>  				return TestFail;
>  			}
> @@ -114,11 +114,11 @@ protected:
>  		const int array[] = { 0, 2, 4, 6, 8 };
>  		i = 0;
>  
> -		for (auto [index, value] : utils::enumerate(array)) {
> -			if (index != i || value != i * 2) {
> +		for (const auto v : utils::enumerate(array)) {
> +			if (v.index != i || v.value != i * 2) {
>  				cerr << "utils::enumerate(<array>) test failed: i=" << i
> -				     << ", index=" << index << ", value=" << value
> -				     << std::endl;
> +				     << ", index=" << v.index << ", value=" << v.value
> +				     << endl;
>  				return TestFail;
>  			}
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund