[libcamera-devel] [PATCH/RFC 2/3] libcamera: utils: enumerate: Use named fields for result
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue May 11 12:30:25 CEST 2021
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 at 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;
>>> }
>>>
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list