[libcamera-devel] [PATCH/RFC 1/3] libcamera: utils: Add enumerate view for range-based for loops
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue May 11 12:14:44 CEST 2021
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 at 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 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?
>> + 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 at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list