| Message ID | 20210423020932.2760-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, thanks for the patch. On Fri, Apr 23, 2021 at 11:09 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Range-based for loops are handy and widely preferred in C++, but are > limited 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. > > 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: > > std::vector<int> values = ...; > > for (auto [index, value] : utils::enumerate(values)) { > ... > } > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/internal/utils.h | 86 ++++++++++++++++++++++++++++++ > src/libcamera/utils.cpp | 29 ++++++++++ > test/utils.cpp | 59 ++++++++++++++++++++ > 3 files changed, 174 insertions(+) > > diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h > index d0146b71727d..9372a75889ac 100644 > --- a/include/libcamera/internal/utils.h > +++ b/include/libcamera/internal/utils.h > @@ -9,12 +9,14 @@ > > #include <algorithm> > #include <chrono> > +#include <iterator> > #include <memory> > #include <ostream> > #include <sstream> > #include <string> > #include <string.h> > #include <sys/time.h> > +#include <utility> > #include <vector> > > #ifndef __DOXYGEN__ > @@ -230,6 +232,90 @@ details::reverse_adapter<T> reverse(T &&iterable) > return { iterable }; > } > > +namespace details { > + > +template<typename Base> > +class enumerate_iterator > +{ I always implemented my own iterator with deriving std::itrator. Today I learned it is getting deprecated... :0 > +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>; > + using pointer = value_type *; > + using reference = value_type &; > + using iterator_category = typename std::iterator_traits<Base>::iterator_category; This class implements only std::forward_iterator_tag. I think this should be std::forward_iterator_tag. > + > + explicit enumerate_iterator(Base iter) > + : current_(iter), pos_(0) > + { > + } > + > + enumerate_iterator &operator++() > + { > + ++current_; > + ++pos_; > + return *this; > + } > + > + bool operator!=(const enumerate_iterator &other) const > + { > + return current_ != other.current_; > + } > + > + value_type operator*() const > + { > + return { pos_, *current_ }; > + } Write in one-line? value_type operator*() const { return { pos_, *current_ }; } > + > +private: > + Base current_; > + difference_type pos_; > +}; > + > +template<typename Base> > +class enumerate_adapter > +{ > +public: > + using iterator = enumerate_iterator<Base>; > + > + enumerate_adapter(Base begin, Base end) > + : begin_(begin), end_(end) > + { > + } > + > + iterator begin() > + { > + return iterator{ begin_ }; > + } > + > + iterator end() > + { > + return iterator{ end_ }; > + } > + begin() and end() should be one-line too. Can we have begin_ and end_ to be iterator, so that begin() and end() just returns them, not dynamically construct iterator? Furthemore, can the variables and the function be constant? -Hiro > +private: > + Base begin_; > + Base end_; > +}; > + > +} /* namespace details */ > + > +template<typename T> > +auto enumerate(T &iterable) -> details::enumerate_adapter<decltype(iterable.begin())> > +{ > + return { std::begin(iterable), std::end(iterable) }; > +} > + > +#ifndef __DOXYGEN__ > +template<typename T, size_t N> > +auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter<T *> > +{ > + return { std::begin(iterable), std::end(iterable) }; > +} > +#endif > + > } /* namespace utils */ > > } /* namespace libcamera */ > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > index c4098a74e0ab..ff9a5832b10e 100644 > --- a/src/libcamera/utils.cpp > +++ b/src/libcamera/utils.cpp > @@ -472,6 +472,35 @@ std::string libcameraSourcePath() > * loop, will cause the loop to iterate over the \a iterable in reverse order > */ > > +/** > + * \fn enumerate(T &iterable) > + * \brief Wrap an iterable to enumerate index and value in a range-based loop > + * \param[in] iterable The iterable > + * > + * Range-based for loops are handy and widely preferred in C++, but are limited > + * 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. > + * > + * 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: > + * > + * \code{.cpp} > + * std::vector<int> values = ...; > + * > + * for (auto [index, value] : utils::enumerate(values)) { > + * ... > + * } > + * \endcode > + * > + * \return A value of unspecified type that, when used in a range-based for > + * loop, iterates over an indexed view of the \a iterable > + */ > + > } /* namespace utils */ > > } /* namespace libcamera */ > diff --git a/test/utils.cpp b/test/utils.cpp > index 08f293898fd9..7e24c71e4775 100644 > --- a/test/utils.cpp > +++ b/test/utils.cpp > @@ -12,6 +12,7 @@ > #include <vector> > > #include <libcamera/geometry.h> > +#include <libcamera/span.h> > > #include "libcamera/internal/utils.h" > > @@ -73,6 +74,60 @@ protected: > return TestPass; > } > > + int testEnumerate() > + { > + 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) { > + cerr << "utils::enumerate(<vector>) test failed: i=" << i > + << ", index=" << index << ", value=" << value > + << std::endl; > + return TestFail; > + } > + > + /* Verify that we can modify the value. */ > + --value; > + ++i; > + } > + > + if (integers != std::vector<int>{ 0, 1, 2, 3, 4 }) { > + cerr << "Failed to modify container in enumerated range loop" << endl; > + return TestFail; > + } > + > + Span<const int> span{ integers }; > + i = 0; > + > + for (auto [index, value] : utils::enumerate(span)) { > + if (index != i || value != i) { > + cerr << "utils::enumerate(<span>) test failed: i=" << i > + << ", index=" << index << ", value=" << value > + << std::endl; > + return TestFail; > + } > + > + ++i; > + } > + > + const int array[] = { 0, 2, 4, 6, 8 }; > + i = 0; > + > + for (auto [index, value] : utils::enumerate(array)) { > + if (index != i || value != i * 2) { > + cerr << "utils::enumerate(<array>) test failed: i=" << i > + << ", index=" << index << ", value=" << value > + << std::endl; > + return TestFail; > + } > + > + ++i; > + } > + > + return TestPass; > + } > + > int run() > { > /* utils::hex() test. */ > @@ -177,6 +232,10 @@ protected: > return TestFail; > } > > + /* utils::enumerate() test. */ > + if (testEnumerate() != TestPass) > + return TestFail; > + > return TestPass; > } > }; > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On 27/04/2021 03:29, Hirokazu Honda wrote: > Hi Laurent, thanks for the patch. > > On Fri, Apr 23, 2021 at 11:09 AM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: >> >> Range-based for loops are handy and widely preferred in C++, but are >> limited 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. >> >> 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: >> >> std::vector<int> values = ...; >> >> for (auto [index, value] : utils::enumerate(values)) { Well that answers my question in 3/3... When used like this, can index be passed by value, and value passed by reference? Given that 'value' is actually supposed to be a reference, would that be a better naming convention? >> ... >> } >> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> --- >> include/libcamera/internal/utils.h | 86 ++++++++++++++++++++++++++++++ >> src/libcamera/utils.cpp | 29 ++++++++++ >> test/utils.cpp | 59 ++++++++++++++++++++ >> 3 files changed, 174 insertions(+) >> >> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h >> index d0146b71727d..9372a75889ac 100644 >> --- a/include/libcamera/internal/utils.h >> +++ b/include/libcamera/internal/utils.h >> @@ -9,12 +9,14 @@ >> >> #include <algorithm> >> #include <chrono> >> +#include <iterator> >> #include <memory> >> #include <ostream> >> #include <sstream> >> #include <string> >> #include <string.h> >> #include <sys/time.h> >> +#include <utility> >> #include <vector> >> >> #ifndef __DOXYGEN__ >> @@ -230,6 +232,90 @@ details::reverse_adapter<T> reverse(T &&iterable) >> return { iterable }; >> } >> >> +namespace details { >> + >> +template<typename Base> >> +class enumerate_iterator >> +{ > > I always implemented my own iterator with deriving std::itrator. > Today I learned it is getting deprecated... :0 > >> +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>; >> + using pointer = value_type *; >> + using reference = value_type &; >> + using iterator_category = typename std::iterator_traits<Base>::iterator_category; > > This class implements only std::forward_iterator_tag. I think this > should be std::forward_iterator_tag. > >> + >> + explicit enumerate_iterator(Base iter) >> + : current_(iter), pos_(0) >> + { >> + } >> + >> + enumerate_iterator &operator++() >> + { >> + ++current_; >> + ++pos_; >> + return *this; >> + } >> + >> + bool operator!=(const enumerate_iterator &other) const >> + { >> + return current_ != other.current_; >> + } >> + >> + value_type operator*() const >> + { >> + return { pos_, *current_ }; >> + } > > Write in one-line? > value_type operator*() const { return { pos_, *current_ }; } > >> + >> +private: >> + Base current_; >> + difference_type pos_; >> +}; >> + >> +template<typename Base> >> +class enumerate_adapter >> +{ >> +public: >> + using iterator = enumerate_iterator<Base>; >> + >> + enumerate_adapter(Base begin, Base end) >> + : begin_(begin), end_(end) >> + { >> + } >> + >> + iterator begin() >> + { >> + return iterator{ begin_ }; >> + } >> + >> + iterator end() >> + { >> + return iterator{ end_ }; >> + } >> + > > begin() and end() should be one-line too. > Can we have begin_ and end_ to be iterator, so that begin() and end() > just returns them, not dynamically construct iterator? > Furthemore, can the variables and the function be constant? > > -Hiro > >> +private: >> + Base begin_; >> + Base end_; >> +}; >> + >> +} /* namespace details */ >> + >> +template<typename T> >> +auto enumerate(T &iterable) -> details::enumerate_adapter<decltype(iterable.begin())> >> +{ >> + return { std::begin(iterable), std::end(iterable) }; >> +} >> + >> +#ifndef __DOXYGEN__ >> +template<typename T, size_t N> >> +auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter<T *> >> +{ >> + return { std::begin(iterable), std::end(iterable) }; >> +} >> +#endif >> + >> } /* namespace utils */ >> >> } /* namespace libcamera */ >> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp >> index c4098a74e0ab..ff9a5832b10e 100644 >> --- a/src/libcamera/utils.cpp >> +++ b/src/libcamera/utils.cpp >> @@ -472,6 +472,35 @@ std::string libcameraSourcePath() >> * loop, will cause the loop to iterate over the \a iterable in reverse order >> */ >> >> +/** >> + * \fn enumerate(T &iterable) >> + * \brief Wrap an iterable to enumerate index and value in a range-based loop >> + * \param[in] iterable The iterable >> + * >> + * Range-based for loops are handy and widely preferred in C++, but are limited >> + * 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. >> + * >> + * 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: >> + * >> + * \code{.cpp} >> + * std::vector<int> values = ...; >> + * >> + * for (auto [index, value] : utils::enumerate(values)) { >> + * ... >> + * } >> + * \endcode >> + * >> + * \return A value of unspecified type that, when used in a range-based for >> + * loop, iterates over an indexed view of the \a iterable >> + */ >> + >> } /* namespace utils */ >> >> } /* namespace libcamera */ >> diff --git a/test/utils.cpp b/test/utils.cpp >> index 08f293898fd9..7e24c71e4775 100644 >> --- a/test/utils.cpp >> +++ b/test/utils.cpp >> @@ -12,6 +12,7 @@ >> #include <vector> >> >> #include <libcamera/geometry.h> >> +#include <libcamera/span.h> >> >> #include "libcamera/internal/utils.h" >> >> @@ -73,6 +74,60 @@ protected: >> return TestPass; >> } >> >> + int testEnumerate() >> + { >> + 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) { >> + cerr << "utils::enumerate(<vector>) test failed: i=" << i >> + << ", index=" << index << ", value=" << value >> + << std::endl; >> + return TestFail; >> + } >> + >> + /* Verify that we can modify the value. */ >> + --value; >> + ++i; >> + } >> + >> + if (integers != std::vector<int>{ 0, 1, 2, 3, 4 }) { >> + cerr << "Failed to modify container in enumerated range loop" << endl; >> + return TestFail; >> + } >> + >> + Span<const int> span{ integers }; >> + i = 0; >> + >> + for (auto [index, value] : utils::enumerate(span)) { >> + if (index != i || value != i) { >> + cerr << "utils::enumerate(<span>) test failed: i=" << i >> + << ", index=" << index << ", value=" << value >> + << std::endl; >> + return TestFail; >> + } >> + >> + ++i; >> + } >> + >> + const int array[] = { 0, 2, 4, 6, 8 }; >> + i = 0; >> + >> + for (auto [index, value] : utils::enumerate(array)) { >> + if (index != i || value != i * 2) { >> + cerr << "utils::enumerate(<array>) test failed: i=" << i >> + << ", index=" << index << ", value=" << value >> + << std::endl; >> + return TestFail; >> + } >> + >> + ++i; >> + } >> + Is there value in adding a test to ensure that if the user increments or otherwise modifies index it will not affect the iteration? Or is index const already? >> + return TestPass; >> + } >> + >> int run() >> { >> /* utils::hex() test. */ >> @@ -177,6 +232,10 @@ protected: >> return TestFail; >> } >> >> + /* utils::enumerate() test. */ >> + if (testEnumerate() != TestPass) >> + return TestFail; >> + >> return TestPass; >> } >> }; >> -- >> Regards, >> >> Laurent Pinchart >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Laurent, Thanks for your work. On 2021-04-23 05:09:30 +0300, Laurent Pinchart wrote: > Range-based for loops are handy and widely preferred in C++, but are > limited 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. > > 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: > > std::vector<int> values = ...; > > for (auto [index, value] : utils::enumerate(values)) { > ... > } > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> This is either really neat or I have written too much python lately :-) Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/internal/utils.h | 86 ++++++++++++++++++++++++++++++ > src/libcamera/utils.cpp | 29 ++++++++++ > test/utils.cpp | 59 ++++++++++++++++++++ > 3 files changed, 174 insertions(+) > > diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h > index d0146b71727d..9372a75889ac 100644 > --- a/include/libcamera/internal/utils.h > +++ b/include/libcamera/internal/utils.h > @@ -9,12 +9,14 @@ > > #include <algorithm> > #include <chrono> > +#include <iterator> > #include <memory> > #include <ostream> > #include <sstream> > #include <string> > #include <string.h> > #include <sys/time.h> > +#include <utility> > #include <vector> > > #ifndef __DOXYGEN__ > @@ -230,6 +232,90 @@ details::reverse_adapter<T> reverse(T &&iterable) > return { iterable }; > } > > +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>; > + using pointer = value_type *; > + using reference = value_type &; > + using iterator_category = typename std::iterator_traits<Base>::iterator_category; > + > + explicit enumerate_iterator(Base iter) > + : current_(iter), pos_(0) > + { > + } > + > + enumerate_iterator &operator++() > + { > + ++current_; > + ++pos_; > + return *this; > + } > + > + bool operator!=(const enumerate_iterator &other) const > + { > + return current_ != other.current_; > + } > + > + value_type operator*() const > + { > + return { pos_, *current_ }; > + } > + > +private: > + Base current_; > + difference_type pos_; > +}; > + > +template<typename Base> > +class enumerate_adapter > +{ > +public: > + using iterator = enumerate_iterator<Base>; > + > + enumerate_adapter(Base begin, Base end) > + : begin_(begin), end_(end) > + { > + } > + > + iterator begin() > + { > + return iterator{ begin_ }; > + } > + > + iterator end() > + { > + return iterator{ end_ }; > + } > + > +private: > + Base begin_; > + Base end_; > +}; > + > +} /* namespace details */ > + > +template<typename T> > +auto enumerate(T &iterable) -> details::enumerate_adapter<decltype(iterable.begin())> > +{ > + return { std::begin(iterable), std::end(iterable) }; > +} > + > +#ifndef __DOXYGEN__ > +template<typename T, size_t N> > +auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter<T *> > +{ > + return { std::begin(iterable), std::end(iterable) }; > +} > +#endif > + > } /* namespace utils */ > > } /* namespace libcamera */ > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > index c4098a74e0ab..ff9a5832b10e 100644 > --- a/src/libcamera/utils.cpp > +++ b/src/libcamera/utils.cpp > @@ -472,6 +472,35 @@ std::string libcameraSourcePath() > * loop, will cause the loop to iterate over the \a iterable in reverse order > */ > > +/** > + * \fn enumerate(T &iterable) > + * \brief Wrap an iterable to enumerate index and value in a range-based loop > + * \param[in] iterable The iterable > + * > + * Range-based for loops are handy and widely preferred in C++, but are limited > + * 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. > + * > + * 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: > + * > + * \code{.cpp} > + * std::vector<int> values = ...; > + * > + * for (auto [index, value] : utils::enumerate(values)) { > + * ... > + * } > + * \endcode > + * > + * \return A value of unspecified type that, when used in a range-based for > + * loop, iterates over an indexed view of the \a iterable > + */ > + > } /* namespace utils */ > > } /* namespace libcamera */ > diff --git a/test/utils.cpp b/test/utils.cpp > index 08f293898fd9..7e24c71e4775 100644 > --- a/test/utils.cpp > +++ b/test/utils.cpp > @@ -12,6 +12,7 @@ > #include <vector> > > #include <libcamera/geometry.h> > +#include <libcamera/span.h> > > #include "libcamera/internal/utils.h" > > @@ -73,6 +74,60 @@ protected: > return TestPass; > } > > + int testEnumerate() > + { > + 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) { > + cerr << "utils::enumerate(<vector>) test failed: i=" << i > + << ", index=" << index << ", value=" << value > + << std::endl; > + return TestFail; > + } > + > + /* Verify that we can modify the value. */ > + --value; > + ++i; > + } > + > + if (integers != std::vector<int>{ 0, 1, 2, 3, 4 }) { > + cerr << "Failed to modify container in enumerated range loop" << endl; > + return TestFail; > + } > + > + Span<const int> span{ integers }; > + i = 0; > + > + for (auto [index, value] : utils::enumerate(span)) { > + if (index != i || value != i) { > + cerr << "utils::enumerate(<span>) test failed: i=" << i > + << ", index=" << index << ", value=" << value > + << std::endl; > + return TestFail; > + } > + > + ++i; > + } > + > + const int array[] = { 0, 2, 4, 6, 8 }; > + i = 0; > + > + for (auto [index, value] : utils::enumerate(array)) { > + if (index != i || value != i * 2) { > + cerr << "utils::enumerate(<array>) test failed: i=" << i > + << ", index=" << index << ", value=" << value > + << std::endl; > + return TestFail; > + } > + > + ++i; > + } > + > + return TestPass; > + } > + > int run() > { > /* utils::hex() test. */ > @@ -177,6 +232,10 @@ protected: > return TestFail; > } > > + /* utils::enumerate() test. */ > + if (testEnumerate() != TestPass) > + return TestFail; > + > return TestPass; > } > }; > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, On Tue, May 11, 2021 at 11:14:44AM +0100, Kieran Bingham wrote: > On 27/04/2021 03:29, Hirokazu Honda wrote: > > On Fri, Apr 23, 2021 at 11:09 AM Laurent Pinchart wrote: > >> > >> Range-based for loops are handy and widely preferred in C++, but are > >> limited 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. > >> > >> 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: > >> > >> std::vector<int> values = ...; > >> > >> for (auto [index, value] : utils::enumerate(values)) { > > Well that answers my question in 3/3... > > When used like this, can index be passed by value, and value passed by > reference? Yes, that's what this patch implements, see enumerate_iterator::value_type returned by operator*() (note that they're not passed, but returned). The index is actually a const value, to ensure that no code will modify it and assume the modification would have a similar effect as modifying the loop counter in a regular for loop. > Given that 'value' is actually supposed to be a reference, would that be > a better naming convention? I'm not sure what you mean here. In any case, this would result in code such as auto cameras = cm_->cameras(); for (auto [index, camera] : utils::enumerate(cameras)) { ... } so there's no variable named 'value'. > >> ... > >> } > >> > >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- > >> include/libcamera/internal/utils.h | 86 ++++++++++++++++++++++++++++++ > >> src/libcamera/utils.cpp | 29 ++++++++++ > >> test/utils.cpp | 59 ++++++++++++++++++++ > >> 3 files changed, 174 insertions(+) > >> > >> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h > >> index d0146b71727d..9372a75889ac 100644 > >> --- a/include/libcamera/internal/utils.h > >> +++ b/include/libcamera/internal/utils.h > >> @@ -9,12 +9,14 @@ > >> > >> #include <algorithm> > >> #include <chrono> > >> +#include <iterator> > >> #include <memory> > >> #include <ostream> > >> #include <sstream> > >> #include <string> > >> #include <string.h> > >> #include <sys/time.h> > >> +#include <utility> > >> #include <vector> > >> > >> #ifndef __DOXYGEN__ > >> @@ -230,6 +232,90 @@ details::reverse_adapter<T> reverse(T &&iterable) > >> return { iterable }; > >> } > >> > >> +namespace details { > >> + > >> +template<typename Base> > >> +class enumerate_iterator > >> +{ > > > > I always implemented my own iterator with deriving std::itrator. > > Today I learned it is getting deprecated... :0 > > > >> +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>; > >> + using pointer = value_type *; > >> + using reference = value_type &; > >> + using iterator_category = typename std::iterator_traits<Base>::iterator_category; > > > > This class implements only std::forward_iterator_tag. I think this > > should be std::forward_iterator_tag. > > > >> + > >> + explicit enumerate_iterator(Base iter) > >> + : current_(iter), pos_(0) > >> + { > >> + } > >> + > >> + enumerate_iterator &operator++() > >> + { > >> + ++current_; > >> + ++pos_; > >> + return *this; > >> + } > >> + > >> + bool operator!=(const enumerate_iterator &other) const > >> + { > >> + return current_ != other.current_; > >> + } > >> + > >> + value_type operator*() const > >> + { > >> + return { pos_, *current_ }; > >> + } > > > > Write in one-line? > > value_type operator*() const { return { pos_, *current_ }; } > > > >> + > >> +private: > >> + Base current_; > >> + difference_type pos_; > >> +}; > >> + > >> +template<typename Base> > >> +class enumerate_adapter > >> +{ > >> +public: > >> + using iterator = enumerate_iterator<Base>; > >> + > >> + enumerate_adapter(Base begin, Base end) > >> + : begin_(begin), end_(end) > >> + { > >> + } > >> + > >> + iterator begin() > >> + { > >> + return iterator{ begin_ }; > >> + } > >> + > >> + iterator end() > >> + { > >> + return iterator{ end_ }; > >> + } > >> + > > > > begin() and end() should be one-line too. > > Can we have begin_ and end_ to be iterator, so that begin() and end() > > just returns them, not dynamically construct iterator? > > Furthemore, can the variables and the function be constant? > > > > -Hiro > > > >> +private: > >> + Base begin_; > >> + Base end_; > >> +}; > >> + > >> +} /* namespace details */ > >> + > >> +template<typename T> > >> +auto enumerate(T &iterable) -> details::enumerate_adapter<decltype(iterable.begin())> > >> +{ > >> + return { std::begin(iterable), std::end(iterable) }; > >> +} > >> + > >> +#ifndef __DOXYGEN__ > >> +template<typename T, size_t N> > >> +auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter<T *> > >> +{ > >> + return { std::begin(iterable), std::end(iterable) }; > >> +} > >> +#endif > >> + > >> } /* namespace utils */ > >> > >> } /* namespace libcamera */ > >> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > >> index c4098a74e0ab..ff9a5832b10e 100644 > >> --- a/src/libcamera/utils.cpp > >> +++ b/src/libcamera/utils.cpp > >> @@ -472,6 +472,35 @@ std::string libcameraSourcePath() > >> * loop, will cause the loop to iterate over the \a iterable in reverse order > >> */ > >> > >> +/** > >> + * \fn enumerate(T &iterable) > >> + * \brief Wrap an iterable to enumerate index and value in a range-based loop > >> + * \param[in] iterable The iterable > >> + * > >> + * Range-based for loops are handy and widely preferred in C++, but are limited > >> + * 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. > >> + * > >> + * 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: > >> + * > >> + * \code{.cpp} > >> + * std::vector<int> values = ...; > >> + * > >> + * for (auto [index, value] : utils::enumerate(values)) { > >> + * ... > >> + * } > >> + * \endcode > >> + * > >> + * \return A value of unspecified type that, when used in a range-based for > >> + * loop, iterates over an indexed view of the \a iterable > >> + */ > >> + > >> } /* namespace utils */ > >> > >> } /* namespace libcamera */ > >> diff --git a/test/utils.cpp b/test/utils.cpp > >> index 08f293898fd9..7e24c71e4775 100644 > >> --- a/test/utils.cpp > >> +++ b/test/utils.cpp > >> @@ -12,6 +12,7 @@ > >> #include <vector> > >> > >> #include <libcamera/geometry.h> > >> +#include <libcamera/span.h> > >> > >> #include "libcamera/internal/utils.h" > >> > >> @@ -73,6 +74,60 @@ protected: > >> return TestPass; > >> } > >> > >> + int testEnumerate() > >> + { > >> + 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) { > >> + cerr << "utils::enumerate(<vector>) test failed: i=" << i > >> + << ", index=" << index << ", value=" << value > >> + << std::endl; > >> + return TestFail; > >> + } > >> + > >> + /* Verify that we can modify the value. */ > >> + --value; > >> + ++i; > >> + } > >> + > >> + if (integers != std::vector<int>{ 0, 1, 2, 3, 4 }) { > >> + cerr << "Failed to modify container in enumerated range loop" << endl; > >> + return TestFail; > >> + } > >> + > >> + Span<const int> span{ integers }; > >> + i = 0; > >> + > >> + for (auto [index, value] : utils::enumerate(span)) { > >> + if (index != i || value != i) { > >> + cerr << "utils::enumerate(<span>) test failed: i=" << i > >> + << ", index=" << index << ", value=" << value > >> + << std::endl; > >> + return TestFail; > >> + } > >> + > >> + ++i; > >> + } > >> + > >> + const int array[] = { 0, 2, 4, 6, 8 }; > >> + i = 0; > >> + > >> + for (auto [index, value] : utils::enumerate(array)) { > >> + if (index != i || value != i * 2) { > >> + cerr << "utils::enumerate(<array>) test failed: i=" << i > >> + << ", index=" << index << ", value=" << value > >> + << std::endl; > >> + return TestFail; > >> + } > >> + > >> + ++i; > >> + } > >> + > > Is there value in adding a test to ensure that if the user increments or > otherwise modifies index it will not affect the iteration? > > Or is index const already? index is const, so that test wouldn't compile :-) ../../test/utils.cpp: In member function ‘int UtilsTest::testEnumerate()’: ../../test/utils.cpp:93:25: error: increment of read-only reference ‘index’ 93 | index++; | ^~~~~ > >> + return TestPass; > >> + } > >> + > >> int run() > >> { > >> /* utils::hex() test. */ > >> @@ -177,6 +232,10 @@ protected: > >> return TestFail; > >> } > >> > >> + /* utils::enumerate() test. */ > >> + if (testEnumerate() != TestPass) > >> + return TestFail; > >> + > >> return TestPass; > >> } > >> };
Hi Hiro, On Tue, Apr 27, 2021 at 11:29:44AM +0900, Hirokazu Honda wrote: > On Fri, Apr 23, 2021 at 11:09 AM Laurent Pinchart wrote: > > > > Range-based for loops are handy and widely preferred in C++, but are > > limited 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. > > > > 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: > > > > std::vector<int> values = ...; > > > > for (auto [index, value] : utils::enumerate(values)) { > > ... > > } > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/internal/utils.h | 86 ++++++++++++++++++++++++++++++ > > src/libcamera/utils.cpp | 29 ++++++++++ > > test/utils.cpp | 59 ++++++++++++++++++++ > > 3 files changed, 174 insertions(+) > > > > diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h > > index d0146b71727d..9372a75889ac 100644 > > --- a/include/libcamera/internal/utils.h > > +++ b/include/libcamera/internal/utils.h > > @@ -9,12 +9,14 @@ > > > > #include <algorithm> > > #include <chrono> > > +#include <iterator> > > #include <memory> > > #include <ostream> > > #include <sstream> > > #include <string> > > #include <string.h> > > #include <sys/time.h> > > +#include <utility> > > #include <vector> > > > > #ifndef __DOXYGEN__ > > @@ -230,6 +232,90 @@ details::reverse_adapter<T> reverse(T &&iterable) > > return { iterable }; > > } > > > > +namespace details { > > + > > +template<typename Base> > > +class enumerate_iterator > > +{ > > I always implemented my own iterator with deriving std::itrator. > Today I learned it is getting deprecated... :0 I've learnt that quite recently too. > > +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>; > > + using pointer = value_type *; > > + using reference = value_type &; > > + using iterator_category = typename std::iterator_traits<Base>::iterator_category; > > This class implements only std::forward_iterator_tag. I think this > should be std::forward_iterator_tag. Good point. I think std::input_iterator_tag would be more appropriate, as the implementation doesn't fulfill the multipass guarantee defined in https://en.cppreference.com/w/cpp/named_req/ForwardIterator. Technically speaking this isn't even a LegacyInputIterator, as it doesn't implement the postfix ++ operator or the -> operator for instance. Doing so may be possible (-> would be tricky as the value_type is currently constructed on-demand), but that wouldn't bring much added value. Still, std::input_iterator_tag is the best match for the intent. > > + > > + explicit enumerate_iterator(Base iter) > > + : current_(iter), pos_(0) > > + { > > + } > > + > > + enumerate_iterator &operator++() > > + { > > + ++current_; > > + ++pos_; > > + return *this; > > + } > > + > > + bool operator!=(const enumerate_iterator &other) const > > + { > > + return current_ != other.current_; > > + } > > + > > + value_type operator*() const > > + { > > + return { pos_, *current_ }; > > + } > > Write in one-line? > value_type operator*() const { return { pos_, *current_ }; } When defining multiple inline functions, with some of them being multi-line, I find it more readable to be consistent and define them all on multiple lines. > > + > > +private: > > + Base current_; > > + difference_type pos_; > > +}; > > + > > +template<typename Base> > > +class enumerate_adapter > > +{ > > +public: > > + using iterator = enumerate_iterator<Base>; > > + > > + enumerate_adapter(Base begin, Base end) > > + : begin_(begin), end_(end) > > + { > > + } > > + > > + iterator begin() > > + { > > + return iterator{ begin_ }; > > + } > > + > > + iterator end() > > + { > > + return iterator{ end_ }; > > + } > > + > > begin() and end() should be one-line too. > Can we have begin_ and end_ to be iterator, so that begin() and end() > just returns them, not dynamically construct iterator? It's doable (and fairly simple), but as we would have to return copies anyway, does it make much of a difference ? > Furthemore, can the variables and the function be constant? Good idea, will be done in v2. > > +private: > > + Base begin_; > > + Base end_; > > +}; > > + > > +} /* namespace details */ > > + > > +template<typename T> > > +auto enumerate(T &iterable) -> details::enumerate_adapter<decltype(iterable.begin())> > > +{ > > + return { std::begin(iterable), std::end(iterable) }; > > +} > > + > > +#ifndef __DOXYGEN__ > > +template<typename T, size_t N> > > +auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter<T *> > > +{ > > + return { std::begin(iterable), std::end(iterable) }; > > +} > > +#endif > > + > > } /* namespace utils */ > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > > index c4098a74e0ab..ff9a5832b10e 100644 > > --- a/src/libcamera/utils.cpp > > +++ b/src/libcamera/utils.cpp > > @@ -472,6 +472,35 @@ std::string libcameraSourcePath() > > * loop, will cause the loop to iterate over the \a iterable in reverse order > > */ > > > > +/** > > + * \fn enumerate(T &iterable) > > + * \brief Wrap an iterable to enumerate index and value in a range-based loop > > + * \param[in] iterable The iterable > > + * > > + * Range-based for loops are handy and widely preferred in C++, but are limited > > + * 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. > > + * > > + * 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: > > + * > > + * \code{.cpp} > > + * std::vector<int> values = ...; > > + * > > + * for (auto [index, value] : utils::enumerate(values)) { > > + * ... > > + * } > > + * \endcode > > + * > > + * \return A value of unspecified type that, when used in a range-based for > > + * loop, iterates over an indexed view of the \a iterable > > + */ > > + > > } /* namespace utils */ > > > > } /* namespace libcamera */ > > diff --git a/test/utils.cpp b/test/utils.cpp > > index 08f293898fd9..7e24c71e4775 100644 > > --- a/test/utils.cpp > > +++ b/test/utils.cpp > > @@ -12,6 +12,7 @@ > > #include <vector> > > > > #include <libcamera/geometry.h> > > +#include <libcamera/span.h> > > > > #include "libcamera/internal/utils.h" > > > > @@ -73,6 +74,60 @@ protected: > > return TestPass; > > } > > > > + int testEnumerate() > > + { > > + 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) { > > + cerr << "utils::enumerate(<vector>) test failed: i=" << i > > + << ", index=" << index << ", value=" << value > > + << std::endl; > > + return TestFail; > > + } > > + > > + /* Verify that we can modify the value. */ > > + --value; > > + ++i; > > + } > > + > > + if (integers != std::vector<int>{ 0, 1, 2, 3, 4 }) { > > + cerr << "Failed to modify container in enumerated range loop" << endl; > > + return TestFail; > > + } > > + > > + Span<const int> span{ integers }; > > + i = 0; > > + > > + for (auto [index, value] : utils::enumerate(span)) { > > + if (index != i || value != i) { > > + cerr << "utils::enumerate(<span>) test failed: i=" << i > > + << ", index=" << index << ", value=" << value > > + << std::endl; > > + return TestFail; > > + } > > + > > + ++i; > > + } > > + > > + const int array[] = { 0, 2, 4, 6, 8 }; > > + i = 0; > > + > > + for (auto [index, value] : utils::enumerate(array)) { > > + if (index != i || value != i * 2) { > > + cerr << "utils::enumerate(<array>) test failed: i=" << i > > + << ", index=" << index << ", value=" << value > > + << std::endl; > > + return TestFail; > > + } > > + > > + ++i; > > + } > > + > > + return TestPass; > > + } > > + > > int run() > > { > > /* utils::hex() test. */ > > @@ -177,6 +232,10 @@ protected: > > return TestFail; > > } > > > > + /* utils::enumerate() test. */ > > + if (testEnumerate() != TestPass) > > + return TestFail; > > + > > return TestPass; > > } > > };
diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h index d0146b71727d..9372a75889ac 100644 --- a/include/libcamera/internal/utils.h +++ b/include/libcamera/internal/utils.h @@ -9,12 +9,14 @@ #include <algorithm> #include <chrono> +#include <iterator> #include <memory> #include <ostream> #include <sstream> #include <string> #include <string.h> #include <sys/time.h> +#include <utility> #include <vector> #ifndef __DOXYGEN__ @@ -230,6 +232,90 @@ details::reverse_adapter<T> reverse(T &&iterable) return { iterable }; } +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>; + using pointer = value_type *; + using reference = value_type &; + using iterator_category = typename std::iterator_traits<Base>::iterator_category; + + explicit enumerate_iterator(Base iter) + : current_(iter), pos_(0) + { + } + + enumerate_iterator &operator++() + { + ++current_; + ++pos_; + return *this; + } + + bool operator!=(const enumerate_iterator &other) const + { + return current_ != other.current_; + } + + value_type operator*() const + { + return { pos_, *current_ }; + } + +private: + Base current_; + difference_type pos_; +}; + +template<typename Base> +class enumerate_adapter +{ +public: + using iterator = enumerate_iterator<Base>; + + enumerate_adapter(Base begin, Base end) + : begin_(begin), end_(end) + { + } + + iterator begin() + { + return iterator{ begin_ }; + } + + iterator end() + { + return iterator{ end_ }; + } + +private: + Base begin_; + Base end_; +}; + +} /* namespace details */ + +template<typename T> +auto enumerate(T &iterable) -> details::enumerate_adapter<decltype(iterable.begin())> +{ + return { std::begin(iterable), std::end(iterable) }; +} + +#ifndef __DOXYGEN__ +template<typename T, size_t N> +auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter<T *> +{ + return { std::begin(iterable), std::end(iterable) }; +} +#endif + } /* namespace utils */ } /* namespace libcamera */ diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp index c4098a74e0ab..ff9a5832b10e 100644 --- a/src/libcamera/utils.cpp +++ b/src/libcamera/utils.cpp @@ -472,6 +472,35 @@ std::string libcameraSourcePath() * loop, will cause the loop to iterate over the \a iterable in reverse order */ +/** + * \fn enumerate(T &iterable) + * \brief Wrap an iterable to enumerate index and value in a range-based loop + * \param[in] iterable The iterable + * + * Range-based for loops are handy and widely preferred in C++, but are limited + * 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. + * + * 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: + * + * \code{.cpp} + * std::vector<int> values = ...; + * + * for (auto [index, value] : utils::enumerate(values)) { + * ... + * } + * \endcode + * + * \return A value of unspecified type that, when used in a range-based for + * loop, iterates over an indexed view of the \a iterable + */ + } /* namespace utils */ } /* namespace libcamera */ diff --git a/test/utils.cpp b/test/utils.cpp index 08f293898fd9..7e24c71e4775 100644 --- a/test/utils.cpp +++ b/test/utils.cpp @@ -12,6 +12,7 @@ #include <vector> #include <libcamera/geometry.h> +#include <libcamera/span.h> #include "libcamera/internal/utils.h" @@ -73,6 +74,60 @@ protected: return TestPass; } + int testEnumerate() + { + 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) { + cerr << "utils::enumerate(<vector>) test failed: i=" << i + << ", index=" << index << ", value=" << value + << std::endl; + return TestFail; + } + + /* Verify that we can modify the value. */ + --value; + ++i; + } + + if (integers != std::vector<int>{ 0, 1, 2, 3, 4 }) { + cerr << "Failed to modify container in enumerated range loop" << endl; + return TestFail; + } + + Span<const int> span{ integers }; + i = 0; + + for (auto [index, value] : utils::enumerate(span)) { + if (index != i || value != i) { + cerr << "utils::enumerate(<span>) test failed: i=" << i + << ", index=" << index << ", value=" << value + << std::endl; + return TestFail; + } + + ++i; + } + + const int array[] = { 0, 2, 4, 6, 8 }; + i = 0; + + for (auto [index, value] : utils::enumerate(array)) { + if (index != i || value != i * 2) { + cerr << "utils::enumerate(<array>) test failed: i=" << i + << ", index=" << index << ", value=" << value + << std::endl; + return TestFail; + } + + ++i; + } + + return TestPass; + } + int run() { /* utils::hex() test. */ @@ -177,6 +232,10 @@ protected: return TestFail; } + /* utils::enumerate() test. */ + if (testEnumerate() != TestPass) + return TestFail; + return TestPass; } };
Range-based for loops are handy and widely preferred in C++, but are limited 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. 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: std::vector<int> values = ...; for (auto [index, value] : utils::enumerate(values)) { ... } Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/internal/utils.h | 86 ++++++++++++++++++++++++++++++ src/libcamera/utils.cpp | 29 ++++++++++ test/utils.cpp | 59 ++++++++++++++++++++ 3 files changed, 174 insertions(+)