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

[Laurent Pinchart]

Hi Hiro,

On Tue, Apr 27, 2021 at 11:30:15AM +0900, Hirokazu Honda wrote:
> Hi Laurent, thanks for the patch.
> 
> Should this patch be squashed with 1/3?

If this patch is accepted, sure. The reason I've posted it separately is
to propose both APIs, I'm not sure yet which one I like really the most.

> On Fri, Apr 23, 2021 at 11:09 AM 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>
> > ---
> >  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