| Message ID | 20210423020932.2760-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, thanks for the patch. Should this patch be squashed with 1/3? On Fri, Apr 23, 2021 at 11:09 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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@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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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; > > } > >
Hi Laurent, On 27/04/2021 04:04, Laurent Pinchart wrote: > 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. Oh I see. So this patch (explicitly) prevents the use of the structured bindings then. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@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; And I think this answers my question previously about is the index const, and it is (both here, and in 1/3). >>> + base_reference value; I still wonder if 'value' is appropriate, as it's a reference, and value could imply pass-by-value. Even if it were 'item' or ... something else? Overall, (I see why you've split these now), I like the idea of moving towards structured bindings, to be able to give appropriate names in the places where they are appropriate. But we haven't been commonly using them so far in libcamera. (Maybe I should go through and fix all the std::pairs that I can). So I certainly prefer the named structure over direct std::pair .first/.seconds, but you haven't suggested using .1st/.2nd ;-) so that's a moot point to me. patch 1/3 would win over this one to me if there were places where using for (auto [index, frob] : utils::enumerate(list)) { std::cout << "- index " << index << ", frob " << frob << std::endl; was explicitly more clear than for (auto frob_wrap : utils::enumerate(list)) { std::cout << "- index " << frob_wrap.index << ", frob " << frob_wrap.value << std::endl; (Yes, I know frob_wrap could be named frob, as your examples, but I am highlighting that this would explicitly show that it's not frob itself....) >>> + }; >>> + >>> +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; >>> } >>> >
Hi Laurent, Thanks for your work. On 2021-04-23 05:09:31 +0300, 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@ideasonboard.com> I really liked the API in 1/3 and my preference would be to keep that. For that reason I do not provide my tag even if I find the patch itself to be good. > --- > 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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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; }
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@ideasonboard.com> --- include/libcamera/internal/utils.h | 16 +++++++++++----- src/libcamera/utils.cpp | 13 +++++++------ test/utils.cpp | 24 ++++++++++++------------ 3 files changed, 30 insertions(+), 23 deletions(-)