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

[Hirokazu Honda]

Hi Laurent, thanks for the patch.

Should this patch be squashed with 1/3?


On Fri, Apr 23, 2021 at 11:09 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> 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>
> ---
>  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