[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 05:55:22 CEST 2021
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 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
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;
> > }
> > };
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list