[libcamera-devel] [PATCH/RFC 1/3] libcamera: utils: Add enumerate view for range-based for loops
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat May 15 04:47:45 CEST 2021
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 at 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;
> >> }
> >> };
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list