[{"id":16598,"web_url":"https://patchwork.libcamera.org/comment/16598/","msgid":"","date":"2021-04-27T02:30:15","subject":"Re: [libcamera-devel] [PATCH/RFC 2/3] libcamera: utils: enumerate:\n\tUse named fields for result","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thanks for the patch.\n\nShould this patch be squashed with 1/3?\n\n\nOn Fri, Apr 23, 2021 at 11:09 AM Laurent Pinchart\n wrote:\n>\n> Returning a pair of { index, value } from container enumeration is\n> error-prone, as the index and value can easily be swapped. Use a\n> structure with named index and value fields instead.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/internal/utils.h | 16 +++++++++++-----\n> src/libcamera/utils.cpp | 13 +++++++------\n> test/utils.cpp | 24 ++++++++++++------------\n> 3 files changed, 30 insertions(+), 23 deletions(-)\n>\n> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n> index 9372a75889ac..54a6b6d283fb 100644\n> --- a/include/libcamera/internal/utils.h\n> +++ b/include/libcamera/internal/utils.h\n> @@ -16,7 +16,6 @@\n> #include \n> #include \n> #include \n> -#include \n> #include \n>\n> #ifndef __DOXYGEN__\n> @@ -237,12 +236,19 @@ namespace details {\n> template\n> class enumerate_iterator\n> {\n> -private:\n> - using base_reference = typename std::iterator_traits::reference;\n> -\n> public:\n> using difference_type = typename std::iterator_traits::difference_type;\n> - using value_type = std::pair;\n> +\n> +private:\n> + using base_reference = typename std::iterator_traits::reference;\n> +\n> + struct result {\n> + const difference_type index;\n> + base_reference value;\n> + };\n> +\n> +public:\n> + using value_type = result;\n> using pointer = value_type *;\n> using reference = value_type &;\n> using iterator_category = typename std::iterator_traits::iterator_category;\n> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> index ff9a5832b10e..b4b4180c1337 100644\n> --- a/src/libcamera/utils.cpp\n> +++ b/src/libcamera/utils.cpp\n> @@ -481,20 +481,21 @@ std::string libcameraSourcePath()\n> * in their ability to replace for loops that require access to a loop counter.\n> * The enumerate() function solves this problem by wrapping the \\a iterable in\n> * an adapter that, when used as a range-expression, will provide iterators\n> - * whose value_type is a pair of index and value reference.\n> + * whose value_type exposes both the element's value and its index.\n> *\n> * The iterable must support std::begin() and std::end(). This includes all\n> * containers provided by the standard C++ library, as well as C-style arrays.\n> *\n> - * A typical usage pattern would use structured binding to store the index and\n> - * value in two separate variables:\n> + * The iterator's value_type is a structure that aggregates the index and value\n> + * reference in two named members. The index is always const, and the value\n> + * reference is conditionally const depending on the iterable. A typical usage\n> + * pattern would be:\n> *\n> * \\code{.cpp}\n> * std::vector values = ...;\n> *\n> - * for (auto [index, value] : utils::enumerate(values)) {\n> - * ...\n> - * }\n> + * for (const auto v : utils::enumerate(values))\n> + * std::cout << \"- index \" << v.index << \", value \" << v.value << std::endl;\n> * \\endcode\n> *\n> * \\return A value of unspecified type that, when used in a range-based for\n> diff --git a/test/utils.cpp b/test/utils.cpp\n> index 7e24c71e4775..06ce5301a74e 100644\n> --- a/test/utils.cpp\n> +++ b/test/utils.cpp\n> @@ -79,16 +79,16 @@ protected:\n> std::vector integers{ 1, 2, 3, 4, 5 };\n> int i = 0;\n>\n> - for (auto [index, value] : utils::enumerate(integers)) {\n> - if (index != i || value != i + 1) {\n> + for (const auto v : utils::enumerate(integers)) {\n> + if (v.index != i || v.value != i + 1) {\n> cerr << \"utils::enumerate() test failed: i=\" << i\n> - << \", index=\" << index << \", value=\" << value\n> - << std::endl;\n> + << \", index=\" << v.index << \", value=\" << v.value\n> + << endl;\n> return TestFail;\n> }\n>\n> /* Verify that we can modify the value. */\n> - --value;\n> + --v.value;\n> ++i;\n> }\n>\n> @@ -100,10 +100,10 @@ protected:\n> Span span{ integers };\n> i = 0;\n>\n> - for (auto [index, value] : utils::enumerate(span)) {\n> - if (index != i || value != i) {\n> + for (const auto v : utils::enumerate(span)) {\n> + if (v.index != i || v.value != i) {\n> cerr << \"utils::enumerate() test failed: i=\" << i\n> - << \", index=\" << index << \", value=\" << value\n> + << \", index=\" << v.index << \", value=\" << v.value\n> << std::endl;\n> return TestFail;\n> }\n> @@ -114,11 +114,11 @@ protected:\n> const int array[] = { 0, 2, 4, 6, 8 };\n> i = 0;\n>\n> - for (auto [index, value] : utils::enumerate(array)) {\n> - if (index != i || value != i * 2) {\n> + for (const auto v : utils::enumerate(array)) {\n> + if (v.index != i || v.value != i * 2) {\n> cerr << \"utils::enumerate() test failed: i=\" << i\n> - << \", index=\" << index << \", value=\" << value\n> - << std::endl;\n> + << \", index=\" << v.index << \", value=\" << v.value\n> + << endl;\n> return TestFail;\n> }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 629EDBDCA1\n\tfor ;\n\tTue, 27 Apr 2021 02:30:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 22C4F68880;\n\tTue, 27 Apr 2021 04:30:27 +0200 (CEST)","from mail-ed1-x531.google.com (mail-ed1-x531.google.com\n\t[IPv6:2a00:1450:4864:20::531])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 25C04605BD\n\tfor ;\n\tTue, 27 Apr 2021 04:30:25 +0200 (CEST)","by mail-ed1-x531.google.com with SMTP id g10so151850edb.0\n\tfor ;\n\tMon, 26 Apr 2021 19:30:25 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"OPvpfg2f\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=4TNwad1c/K/5BTQsQWt4dIJUPrKJA2AQL/gREo6kios=;\n\tb=OPvpfg2f9qtJ8iC8u7ZycFHJX34xG69xYDJ+6m8i53pURygG900+3BnwZDi9OGjYt1\n\tA6BAenwwWICEv3u/1HZEXo7U9/Gsyp50d42rwwqabQXw56dwNCldlFJAxHXnRlzzwUop\n\tc4ZvOiv89uSHsKFC6Y+qZaf5A4xiVBp8q5D0I=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=4TNwad1c/K/5BTQsQWt4dIJUPrKJA2AQL/gREo6kios=;\n\tb=VC6oNLprpgPaGf7+Xg7EXI46JUtt7znJbeltWJFvMDBlZ9RE7fDGxz4Dtmwt2Q9C7i\n\tQQI1ONcEq5PAvH/5HHeQfULpucLZPTSFGm2RAHkXe9vs8z6lXoWMXzAwX629fb1uXor8\n\tAWWMkDDXXDWi61CH3ar+SzjQQs33zioYvs3NnLkqUpNpSwffj/XZ+pW75vsQmYMiHopV\n\tDAu41j4SYwz0ZAJfPwOVqS8AaaKtpDMydomD9Fc305XxpBzW7Q7V8i3El2PC6nWZ5pOJ\n\td1qPTe5O36EreC6fOC+tAsSRlPyuru/bP375I4gKD3kNrURuU5sRyi4JVLP96ste9/Js\n\tTolA==","X-Gm-Message-State":"AOAM531GGm5gr/Vgda4nCF6n9uNYEs1ppvGlu3Fk0tqAtT38GUsnWdVo\n\tLovcsbk+YRvOhOCxkewjRtTQtVQ5JXAK25Kqfig1SltAM+c=","X-Google-Smtp-Source":"ABdhPJy+e+GtulUVuMLEo3h7gJz21kHmaAxO2ukCWBKwETEpe8O+H1F7tcLhmejkqqOc/hVV6VqwpamvNhJrE+lLqkE=","X-Received":"by 2002:a05:6402:488:: with SMTP id\n\tk8mr1700820edv.233.1619490624764; \n\tMon, 26 Apr 2021 19:30:24 -0700 (PDT)","MIME-Version":"1.0","References":"<20210423020932.2760-1-laurent.pinchart@ideasonboard.com>\n\t<20210423020932.2760-3-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20210423020932.2760-3-laurent.pinchart@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Tue, 27 Apr 2021 11:30:15 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH/RFC 2/3] libcamera: utils: enumerate:\n\tUse named fields for result","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16600,"web_url":"https://patchwork.libcamera.org/comment/16600/","msgid":"","date":"2021-04-27T03:04:08","subject":"Re: [libcamera-devel] [PATCH/RFC 2/3] libcamera: utils: enumerate:\n\tUse named fields for result","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Tue, Apr 27, 2021 at 11:30:15AM +0900, Hirokazu Honda wrote:\n> Hi Laurent, thanks for the patch.\n> \n> Should this patch be squashed with 1/3?\n\nIf this patch is accepted, sure. The reason I've posted it separately is\nto propose both APIs, I'm not sure yet which one I like really the most.\n\n> On Fri, Apr 23, 2021 at 11:09 AM Laurent Pinchart wrote:\n> >\n> > Returning a pair of { index, value } from container enumeration is\n> > error-prone, as the index and value can easily be swapped. Use a\n> > structure with named index and value fields instead.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/internal/utils.h | 16 +++++++++++-----\n> > src/libcamera/utils.cpp | 13 +++++++------\n> > test/utils.cpp | 24 ++++++++++++------------\n> > 3 files changed, 30 insertions(+), 23 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n> > index 9372a75889ac..54a6b6d283fb 100644\n> > --- a/include/libcamera/internal/utils.h\n> > +++ b/include/libcamera/internal/utils.h\n> > @@ -16,7 +16,6 @@\n> > #include \n> > #include \n> > #include \n> > -#include \n> > #include \n> >\n> > #ifndef __DOXYGEN__\n> > @@ -237,12 +236,19 @@ namespace details {\n> > template\n> > class enumerate_iterator\n> > {\n> > -private:\n> > - using base_reference = typename std::iterator_traits::reference;\n> > -\n> > public:\n> > using difference_type = typename std::iterator_traits::difference_type;\n> > - using value_type = std::pair;\n> > +\n> > +private:\n> > + using base_reference = typename std::iterator_traits::reference;\n> > +\n> > + struct result {\n> > + const difference_type index;\n> > + base_reference value;\n> > + };\n> > +\n> > +public:\n> > + using value_type = result;\n> > using pointer = value_type *;\n> > using reference = value_type &;\n> > using iterator_category = typename std::iterator_traits::iterator_category;\n> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > index ff9a5832b10e..b4b4180c1337 100644\n> > --- a/src/libcamera/utils.cpp\n> > +++ b/src/libcamera/utils.cpp\n> > @@ -481,20 +481,21 @@ std::string libcameraSourcePath()\n> > * in their ability to replace for loops that require access to a loop counter.\n> > * The enumerate() function solves this problem by wrapping the \\a iterable in\n> > * an adapter that, when used as a range-expression, will provide iterators\n> > - * whose value_type is a pair of index and value reference.\n> > + * whose value_type exposes both the element's value and its index.\n> > *\n> > * The iterable must support std::begin() and std::end(). This includes all\n> > * containers provided by the standard C++ library, as well as C-style arrays.\n> > *\n> > - * A typical usage pattern would use structured binding to store the index and\n> > - * value in two separate variables:\n> > + * The iterator's value_type is a structure that aggregates the index and value\n> > + * reference in two named members. The index is always const, and the value\n> > + * reference is conditionally const depending on the iterable. A typical usage\n> > + * pattern would be:\n> > *\n> > * \\code{.cpp}\n> > * std::vector values = ...;\n> > *\n> > - * for (auto [index, value] : utils::enumerate(values)) {\n> > - * ...\n> > - * }\n> > + * for (const auto v : utils::enumerate(values))\n> > + * std::cout << \"- index \" << v.index << \", value \" << v.value << std::endl;\n> > * \\endcode\n> > *\n> > * \\return A value of unspecified type that, when used in a range-based for\n> > diff --git a/test/utils.cpp b/test/utils.cpp\n> > index 7e24c71e4775..06ce5301a74e 100644\n> > --- a/test/utils.cpp\n> > +++ b/test/utils.cpp\n> > @@ -79,16 +79,16 @@ protected:\n> > std::vector integers{ 1, 2, 3, 4, 5 };\n> > int i = 0;\n> >\n> > - for (auto [index, value] : utils::enumerate(integers)) {\n> > - if (index != i || value != i + 1) {\n> > + for (const auto v : utils::enumerate(integers)) {\n> > + if (v.index != i || v.value != i + 1) {\n> > cerr << \"utils::enumerate() test failed: i=\" << i\n> > - << \", index=\" << index << \", value=\" << value\n> > - << std::endl;\n> > + << \", index=\" << v.index << \", value=\" << v.value\n> > + << endl;\n> > return TestFail;\n> > }\n> >\n> > /* Verify that we can modify the value. */\n> > - --value;\n> > + --v.value;\n> > ++i;\n> > }\n> >\n> > @@ -100,10 +100,10 @@ protected:\n> > Span span{ integers };\n> > i = 0;\n> >\n> > - for (auto [index, value] : utils::enumerate(span)) {\n> > - if (index != i || value != i) {\n> > + for (const auto v : utils::enumerate(span)) {\n> > + if (v.index != i || v.value != i) {\n> > cerr << \"utils::enumerate() test failed: i=\" << i\n> > - << \", index=\" << index << \", value=\" << value\n> > + << \", index=\" << v.index << \", value=\" << v.value\n> > << std::endl;\n> > return TestFail;\n> > }\n> > @@ -114,11 +114,11 @@ protected:\n> > const int array[] = { 0, 2, 4, 6, 8 };\n> > i = 0;\n> >\n> > - for (auto [index, value] : utils::enumerate(array)) {\n> > - if (index != i || value != i * 2) {\n> > + for (const auto v : utils::enumerate(array)) {\n> > + if (v.index != i || v.value != i * 2) {\n> > cerr << \"utils::enumerate() test failed: i=\" << i\n> > - << \", index=\" << index << \", value=\" << value\n> > - << std::endl;\n> > + << \", index=\" << v.index << \", value=\" << v.value\n> > + << endl;\n> > return TestFail;\n> > }\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 45AE1BDCA1\n\tfor ;\n\tTue, 27 Apr 2021 03:04:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC89F68878;\n\tTue, 27 Apr 2021 05:04:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2560A60512\n\tfor ;\n\tTue, 27 Apr 2021 05:04:15 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 90856E9;\n\tTue, 27 Apr 2021 05:04:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"irM/Z11Q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619492654;\n\tbh=Zo9RPwOCjiRK3dZp6zQ1IlOiBsg/cCgbdFfWMBV+amI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=irM/Z11Q5ij0uNwfAdv+2b3+QQVqMKMK+EN+nSBQZLBBxJBm3MRxIF6hmqZKNjflp\n\t0cR+L8ofrwrcP34IRujPXpzvfVXm2MQHVUyU8wTuhmQCZsfXm32o4jcQ7k9zbeinlK\n\tqCKXmifVtgW67MtyAccg1zCxvTYM+iOiBMRgk2tA=","Date":"Tue, 27 Apr 2021 06:04:08 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210423020932.2760-1-laurent.pinchart@ideasonboard.com>\n\t<20210423020932.2760-3-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH/RFC 2/3] libcamera: utils: enumerate:\n\tUse named fields for result","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16893,"web_url":"https://patchwork.libcamera.org/comment/16893/","msgid":"","date":"2021-05-11T10:30:25","subject":"Re: [libcamera-devel] [PATCH/RFC 2/3] libcamera: utils: enumerate:\n\tUse named fields for result","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 27/04/2021 04:04, Laurent Pinchart wrote:\n> Hi Hiro,\n> \n> On Tue, Apr 27, 2021 at 11:30:15AM +0900, Hirokazu Honda wrote:\n>> Hi Laurent, thanks for the patch.\n>>\n>> Should this patch be squashed with 1/3?\n> \n> If this patch is accepted, sure. The reason I've posted it separately is\n> to propose both APIs, I'm not sure yet which one I like really the most.\n> \n>> On Fri, Apr 23, 2021 at 11:09 AM Laurent Pinchart wrote:\n>>>\n>>> Returning a pair of { index, value } from container enumeration is\n>>> error-prone, as the index and value can easily be swapped. Use a\n>>> structure with named index and value fields instead.\n\nOh I see.\n\nSo this patch (explicitly) prevents the use of the structured bindings then.\n\n\n>>>\n>>> Signed-off-by: Laurent Pinchart \n>>> ---\n>>> include/libcamera/internal/utils.h | 16 +++++++++++-----\n>>> src/libcamera/utils.cpp | 13 +++++++------\n>>> test/utils.cpp | 24 ++++++++++++------------\n>>> 3 files changed, 30 insertions(+), 23 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n>>> index 9372a75889ac..54a6b6d283fb 100644\n>>> --- a/include/libcamera/internal/utils.h\n>>> +++ b/include/libcamera/internal/utils.h\n>>> @@ -16,7 +16,6 @@\n>>> #include \n>>> #include \n>>> #include \n>>> -#include \n>>> #include \n>>>\n>>> #ifndef __DOXYGEN__\n>>> @@ -237,12 +236,19 @@ namespace details {\n>>> template\n>>> class enumerate_iterator\n>>> {\n>>> -private:\n>>> - using base_reference = typename std::iterator_traits::reference;\n>>> -\n>>> public:\n>>> using difference_type = typename std::iterator_traits::difference_type;\n>>> - using value_type = std::pair;\n>>> +\n>>> +private:\n>>> + using base_reference = typename std::iterator_traits::reference;\n>>> +\n>>> + struct result {\n>>> + const difference_type index;\n\nAnd I think this answers my question previously about is the index\nconst, and it is (both here, and in 1/3).\n\n\n>>> + base_reference value;\n\nI still wonder if 'value' is appropriate, as it's a reference, and value\ncould imply pass-by-value.\n\nEven if it were 'item' or ... something else?\n\n\nOverall, (I see why you've split these now), I like the idea of moving\ntowards structured bindings, to be able to give appropriate names in the\nplaces where they are appropriate. But we haven't been commonly using\nthem so far in libcamera. (Maybe I should go through and fix all the\nstd::pairs that I can).\n\nSo I certainly prefer the named structure over direct std::pair\n.first/.seconds, but you haven't suggested using .1st/.2nd ;-) so that's\na moot point to me.\n\npatch 1/3 would win over this one to me if there were places where using\n for (auto [index, frob] : utils::enumerate(list)) {\n\tstd::cout << \"- index \" << index\n\t\t << \", frob \" << frob\n\t\t << std::endl;\n\nwas explicitly more clear than\n\n for (auto frob_wrap : utils::enumerate(list)) {\n\tstd::cout << \"- index \" << frob_wrap.index\n\t\t << \", frob \" << frob_wrap.value\n\t\t << std::endl;\n\n(Yes, I know frob_wrap could be named frob, as your examples, but I am\nhighlighting that this would explicitly show that it's not frob itself....)\n\n\n>>> + };\n>>> +\n>>> +public:\n>>> + using value_type = result;\n>>> using pointer = value_type *;\n>>> using reference = value_type &;\n>>> using iterator_category = typename std::iterator_traits::iterator_category;\n>>> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n>>> index ff9a5832b10e..b4b4180c1337 100644\n>>> --- a/src/libcamera/utils.cpp\n>>> +++ b/src/libcamera/utils.cpp\n>>> @@ -481,20 +481,21 @@ std::string libcameraSourcePath()\n>>> * in their ability to replace for loops that require access to a loop counter.\n>>> * The enumerate() function solves this problem by wrapping the \\a iterable in\n>>> * an adapter that, when used as a range-expression, will provide iterators\n>>> - * whose value_type is a pair of index and value reference.\n>>> + * whose value_type exposes both the element's value and its index.\n>>> *\n>>> * The iterable must support std::begin() and std::end(). This includes all\n>>> * containers provided by the standard C++ library, as well as C-style arrays.\n>>> *\n>>> - * A typical usage pattern would use structured binding to store the index and\n>>> - * value in two separate variables:\n>>> + * The iterator's value_type is a structure that aggregates the index and value\n>>> + * reference in two named members. The index is always const, and the value\n>>> + * reference is conditionally const depending on the iterable. A typical usage\n>>> + * pattern would be:\n>>> *\n>>> * \\code{.cpp}\n>>> * std::vector values = ...;\n>>> *\n>>> - * for (auto [index, value] : utils::enumerate(values)) {\n>>> - * ...\n>>> - * }\n>>> + * for (const auto v : utils::enumerate(values))\n>>> + * std::cout << \"- index \" << v.index << \", value \" << v.value << std::endl;\n>>> * \\endcode\n>>> *\n>>> * \\return A value of unspecified type that, when used in a range-based for\n>>> diff --git a/test/utils.cpp b/test/utils.cpp\n>>> index 7e24c71e4775..06ce5301a74e 100644\n>>> --- a/test/utils.cpp\n>>> +++ b/test/utils.cpp\n>>> @@ -79,16 +79,16 @@ protected:\n>>> std::vector integers{ 1, 2, 3, 4, 5 };\n>>> int i = 0;\n>>>\n>>> - for (auto [index, value] : utils::enumerate(integers)) {\n>>> - if (index != i || value != i + 1) {\n>>> + for (const auto v : utils::enumerate(integers)) {\n>>> + if (v.index != i || v.value != i + 1) {\n>>> cerr << \"utils::enumerate() test failed: i=\" << i\n>>> - << \", index=\" << index << \", value=\" << value\n>>> - << std::endl;\n>>> + << \", index=\" << v.index << \", value=\" << v.value\n>>> + << endl;\n>>> return TestFail;\n>>> }\n>>>\n>>> /* Verify that we can modify the value. */\n>>> - --value;\n>>> + --v.value;\n>>> ++i;\n>>> }\n>>>\n>>> @@ -100,10 +100,10 @@ protected:\n>>> Span span{ integers };\n>>> i = 0;\n>>>\n>>> - for (auto [index, value] : utils::enumerate(span)) {\n>>> - if (index != i || value != i) {\n>>> + for (const auto v : utils::enumerate(span)) {\n>>> + if (v.index != i || v.value != i) {\n>>> cerr << \"utils::enumerate() test failed: i=\" << i\n>>> - << \", index=\" << index << \", value=\" << value\n>>> + << \", index=\" << v.index << \", value=\" << v.value\n>>> << std::endl;\n>>> return TestFail;\n>>> }\n>>> @@ -114,11 +114,11 @@ protected:\n>>> const int array[] = { 0, 2, 4, 6, 8 };\n>>> i = 0;\n>>>\n>>> - for (auto [index, value] : utils::enumerate(array)) {\n>>> - if (index != i || value != i * 2) {\n>>> + for (const auto v : utils::enumerate(array)) {\n>>> + if (v.index != i || v.value != i * 2) {\n>>> cerr << \"utils::enumerate() test failed: i=\" << i\n>>> - << \", index=\" << index << \", value=\" << value\n>>> - << std::endl;\n>>> + << \", index=\" << v.index << \", value=\" << v.value\n>>> + << endl;\n>>> return TestFail;\n>>> }\n>>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 61167BF839\n\tfor ;\n\tTue, 11 May 2021 10:30:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B861D68918;\n\tTue, 11 May 2021 12:30:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CAC4461538\n\tfor ;\n\tTue, 11 May 2021 12:30:28 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 345534A2;\n\tTue, 11 May 2021 12:30:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OhxGW/X7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620729028;\n\tbh=vfKw3pfalGQkTHgLw2TTnc2hIJeWH3ShXwmpLXvl1Xg=;\n\th=Reply-To:To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=OhxGW/X7zYDjNMJ7H9nBzZNbfwYzNRCt6djvvnkEgKqrVZTrVPYW/9npdJs8I0Otx\n\tNoHPiK/pKwrjHZXISBlwgq7GQeR5d9Ywqp3FN0rTGfBRCSMQK29UfWUr3nQCLeQyxt\n\tHDAa85AwzlkQD4fxZimQsq4LH8D4Bp5ni4XGRaTE=","To":"Laurent Pinchart ,\n\tHirokazu Honda ","References":"<20210423020932.2760-1-laurent.pinchart@ideasonboard.com>\n\t<20210423020932.2760-3-laurent.pinchart@ideasonboard.com>\n\t\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"","Date":"Tue, 11 May 2021 11:30:25 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH/RFC 2/3] libcamera: utils: enumerate:\n\tUse named fields for result","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16899,"web_url":"https://patchwork.libcamera.org/comment/16899/","msgid":"","date":"2021-05-11T11:41:43","subject":"Re: [libcamera-devel] [PATCH/RFC 2/3] libcamera: utils: enumerate:\n\tUse named fields for result","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2021-04-23 05:09:31 +0300, Laurent Pinchart wrote:\n> Returning a pair of { index, value } from container enumeration is\n> error-prone, as the index and value can easily be swapped. Use a\n> structure with named index and value fields instead.\n> \n> Signed-off-by: Laurent Pinchart \n\nI really liked the API in 1/3 and my preference would be to keep that. \nFor that reason I do not provide my tag even if I find the patch itself \nto be good.\n\n> ---\n> include/libcamera/internal/utils.h | 16 +++++++++++-----\n> src/libcamera/utils.cpp | 13 +++++++------\n> test/utils.cpp | 24 ++++++++++++------------\n> 3 files changed, 30 insertions(+), 23 deletions(-)\n> \n> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n> index 9372a75889ac..54a6b6d283fb 100644\n> --- a/include/libcamera/internal/utils.h\n> +++ b/include/libcamera/internal/utils.h\n> @@ -16,7 +16,6 @@\n> #include \n> #include \n> #include \n> -#include \n> #include \n> \n> #ifndef __DOXYGEN__\n> @@ -237,12 +236,19 @@ namespace details {\n> template\n> class enumerate_iterator\n> {\n> -private:\n> -\tusing base_reference = typename std::iterator_traits::reference;\n> -\n> public:\n> \tusing difference_type = typename std::iterator_traits::difference_type;\n> -\tusing value_type = std::pair;\n> +\n> +private:\n> +\tusing base_reference = typename std::iterator_traits::reference;\n> +\n> +\tstruct result {\n> +\t\tconst difference_type index;\n> +\t\tbase_reference value;\n> +\t};\n> +\n> +public:\n> +\tusing value_type = result;\n> \tusing pointer = value_type *;\n> \tusing reference = value_type &;\n> \tusing iterator_category = typename std::iterator_traits::iterator_category;\n> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> index ff9a5832b10e..b4b4180c1337 100644\n> --- a/src/libcamera/utils.cpp\n> +++ b/src/libcamera/utils.cpp\n> @@ -481,20 +481,21 @@ std::string libcameraSourcePath()\n> * in their ability to replace for loops that require access to a loop counter.\n> * The enumerate() function solves this problem by wrapping the \\a iterable in\n> * an adapter that, when used as a range-expression, will provide iterators\n> - * whose value_type is a pair of index and value reference.\n> + * whose value_type exposes both the element's value and its index.\n> *\n> * The iterable must support std::begin() and std::end(). This includes all\n> * containers provided by the standard C++ library, as well as C-style arrays.\n> *\n> - * A typical usage pattern would use structured binding to store the index and\n> - * value in two separate variables:\n> + * The iterator's value_type is a structure that aggregates the index and value\n> + * reference in two named members. The index is always const, and the value\n> + * reference is conditionally const depending on the iterable. A typical usage\n> + * pattern would be:\n> *\n> * \\code{.cpp}\n> * std::vector values = ...;\n> *\n> - * for (auto [index, value] : utils::enumerate(values)) {\n> - * \t...\n> - * }\n> + * for (const auto v : utils::enumerate(values))\n> + * \tstd::cout << \"- index \" << v.index << \", value \" << v.value << std::endl;\n> * \\endcode\n> *\n> * \\return A value of unspecified type that, when used in a range-based for\n> diff --git a/test/utils.cpp b/test/utils.cpp\n> index 7e24c71e4775..06ce5301a74e 100644\n> --- a/test/utils.cpp\n> +++ b/test/utils.cpp\n> @@ -79,16 +79,16 @@ protected:\n> \t\tstd::vector integers{ 1, 2, 3, 4, 5 };\n> \t\tint i = 0;\n> \n> -\t\tfor (auto [index, value] : utils::enumerate(integers)) {\n> -\t\t\tif (index != i || value != i + 1) {\n> +\t\tfor (const auto v : utils::enumerate(integers)) {\n> +\t\t\tif (v.index != i || v.value != i + 1) {\n> \t\t\t\tcerr << \"utils::enumerate() test failed: i=\" << i\n> -\t\t\t\t << \", index=\" << index << \", value=\" << value\n> -\t\t\t\t << std::endl;\n> +\t\t\t\t << \", index=\" << v.index << \", value=\" << v.value\n> +\t\t\t\t << endl;\n> \t\t\t\treturn TestFail;\n> \t\t\t}\n> \n> \t\t\t/* Verify that we can modify the value. */\n> -\t\t\t--value;\n> +\t\t\t--v.value;\n> \t\t\t++i;\n> \t\t}\n> \n> @@ -100,10 +100,10 @@ protected:\n> \t\tSpan span{ integers };\n> \t\ti = 0;\n> \n> -\t\tfor (auto [index, value] : utils::enumerate(span)) {\n> -\t\t\tif (index != i || value != i) {\n> +\t\tfor (const auto v : utils::enumerate(span)) {\n> +\t\t\tif (v.index != i || v.value != i) {\n> \t\t\t\tcerr << \"utils::enumerate() test failed: i=\" << i\n> -\t\t\t\t << \", index=\" << index << \", value=\" << value\n> +\t\t\t\t << \", index=\" << v.index << \", value=\" << v.value\n> \t\t\t\t << std::endl;\n> \t\t\t\treturn TestFail;\n> \t\t\t}\n> @@ -114,11 +114,11 @@ protected:\n> \t\tconst int array[] = { 0, 2, 4, 6, 8 };\n> \t\ti = 0;\n> \n> -\t\tfor (auto [index, value] : utils::enumerate(array)) {\n> -\t\t\tif (index != i || value != i * 2) {\n> +\t\tfor (const auto v : utils::enumerate(array)) {\n> +\t\t\tif (v.index != i || v.value != i * 2) {\n> \t\t\t\tcerr << \"utils::enumerate() test failed: i=\" << i\n> -\t\t\t\t << \", index=\" << index << \", value=\" << value\n> -\t\t\t\t << std::endl;\n> +\t\t\t\t << \", index=\" << v.index << \", value=\" << v.value\n> +\t\t\t\t << endl;\n> \t\t\t\treturn TestFail;\n> \t\t\t}\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 38259BF829\n\tfor ;\n\tTue, 11 May 2021 11:41:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9233068918;\n\tTue, 11 May 2021 13:41:45 +0200 (CEST)","from mail-lf1-x132.google.com (mail-lf1-x132.google.com\n\t[IPv6:2a00:1450:4864:20::132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A961161538\n\tfor ;\n\tTue, 11 May 2021 13:41:44 +0200 (CEST)","by mail-lf1-x132.google.com with SMTP id x19so28198879lfa.2\n\tfor ;\n\tTue, 11 May 2021 04:41:44 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tz41sm2605712lfu.88.2021.05.11.04.41.43\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 11 May 2021 04:41:43 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"ytf000JJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=ezgQXhRxjt9lz7BpaXAVBf+WZOmLRnVE0tzEX8F9v3E=;\n\tb=ytf000JJexlVRKpX5VcDt+3DO33rc3105hFKQRlEe1F1CKkQFN9RPx12HjMPzLWNxQ\n\tHg6oBB2Hts3yDwAdg5aXn1ENRjfp7ryq0we8MKWLOSqoyql++f9P8Il8VJYeturZu01n\n\t9//34tKYSOdXPkyl3bQgCzP/qtnAWE7Ihnu/cF+0+NLfGg9BuAjL/+pOvDsig+HhsiNB\n\tm3+KoX9vCBvlrIiERTkJNJk0IFGWF2d+MmF1uKQS+oj7paYrpY2KieIGexGkB+xA7m/3\n\tltzMQxzK0h+wf++NKf3vI475a8O1kfMx0dqBvAClzsKClT6VrVGQ4cGmoP37RYlaEi36\n\tbdsA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=ezgQXhRxjt9lz7BpaXAVBf+WZOmLRnVE0tzEX8F9v3E=;\n\tb=uWQT36d7sLcaIv3O/lrTT3T16JGn62UqDl5RgT0LdJxBrM3vQTL2qtyoWqe5t2sKTH\n\t9Si+hBukZLoB8FuI7XyO6iDQkRNGC1WYTFKHyNuJ0cTaJjfraN//2Lvtk/BdxvlvOxXJ\n\tSvDjJI5VGDlmLLIE2ZJWXXO9PnpGdR7DoGGQJVCbKzfaHmRjjAZMCnw5aAn+3IZXjkcI\n\tMprnJsyWjeBIl50rhai/fZLy9Fdh78i+lGnQ6ToHy3k6hkR53S2EngeyWENFkJb3Hob7\n\tPN726mS9oJYnMoWHAN3cqA/cXYiqDM4RISMidfvkW0lVjwaKWZUGHB/88pDHkYEP5Tf5\n\tZ22A==","X-Gm-Message-State":"AOAM530dTwHVdGnG0f/ygsxSCrnyYP6tgzn7ZEF8oKRQBFL9kED51gZg\n\t7CJK8T0A9kLAq738zBjvzT2O7YsYamBYhY0I","X-Google-Smtp-Source":"ABdhPJxHLtEZ5156VyS51so9p5bGzntzQlsUQ4ci5ZJM4FatF9Z9TMzoOPGcTeyKo7xpKUED68uEVA==","X-Received":"by 2002:a05:6512:1116:: with SMTP id\n\tl22mr20713713lfg.473.1620733304072; \n\tTue, 11 May 2021 04:41:44 -0700 (PDT)","Date":"Tue, 11 May 2021 13:41:43 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"","References":"<20210423020932.2760-1-laurent.pinchart@ideasonboard.com>\n\t<20210423020932.2760-3-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210423020932.2760-3-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH/RFC 2/3] libcamera: utils: enumerate:\n\tUse named fields for result","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]