[{"id":16597,"web_url":"https://patchwork.libcamera.org/comment/16597/","msgid":"","date":"2021-04-27T02:29:44","subject":"Re: [libcamera-devel] [PATCH/RFC 1/3] libcamera: utils: Add\n\tenumerate view for range-based for loops","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\nOn Fri, Apr 23, 2021 at 11:09 AM Laurent Pinchart\n wrote:\n>\n> Range-based for loops are handy and widely preferred in C++, but are\n> limited in their ability to replace for loops that require access to a\n> loop counter. The enumerate() function solves this problem by wrapping\n> the \\a iterable in an adapter that, when used as a range-expression,\n> will provide iterators whose value_type is a pair of index and value\n> reference.\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\n> arrays.\n>\n> A typical usage pattern would use structured binding to store the index\n> and value in two separate variables:\n>\n> std::vector values = ...;\n>\n> for (auto [index, value] : utils::enumerate(values)) {\n> ...\n> }\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/internal/utils.h | 86 ++++++++++++++++++++++++++++++\n> src/libcamera/utils.cpp | 29 ++++++++++\n> test/utils.cpp | 59 ++++++++++++++++++++\n> 3 files changed, 174 insertions(+)\n>\n> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n> index d0146b71727d..9372a75889ac 100644\n> --- a/include/libcamera/internal/utils.h\n> +++ b/include/libcamera/internal/utils.h\n> @@ -9,12 +9,14 @@\n>\n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> #include \n> #include \n> #include \n> +#include \n> #include \n>\n> #ifndef __DOXYGEN__\n> @@ -230,6 +232,90 @@ details::reverse_adapter reverse(T &&iterable)\n> return { iterable };\n> }\n>\n> +namespace details {\n> +\n> +template\n> +class enumerate_iterator\n> +{\n\nI always implemented my own iterator with deriving std::itrator.\nToday I learned it is getting deprecated... :0\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> + using pointer = value_type *;\n> + using reference = value_type &;\n> + using iterator_category = typename std::iterator_traits::iterator_category;\n\nThis class implements only std::forward_iterator_tag. I think this\nshould be std::forward_iterator_tag.\n\n> +\n> + explicit enumerate_iterator(Base iter)\n> + : current_(iter), pos_(0)\n> + {\n> + }\n> +\n> + enumerate_iterator &operator++()\n> + {\n> + ++current_;\n> + ++pos_;\n> + return *this;\n> + }\n> +\n> + bool operator!=(const enumerate_iterator &other) const\n> + {\n> + return current_ != other.current_;\n> + }\n> +\n> + value_type operator*() const\n> + {\n> + return { pos_, *current_ };\n> + }\n\nWrite in one-line?\nvalue_type operator*() const { return { pos_, *current_ }; }\n\n> +\n> +private:\n> + Base current_;\n> + difference_type pos_;\n> +};\n> +\n> +template\n> +class enumerate_adapter\n> +{\n> +public:\n> + using iterator = enumerate_iterator;\n> +\n> + enumerate_adapter(Base begin, Base end)\n> + : begin_(begin), end_(end)\n> + {\n> + }\n> +\n> + iterator begin()\n> + {\n> + return iterator{ begin_ };\n> + }\n> +\n> + iterator end()\n> + {\n> + return iterator{ end_ };\n> + }\n> +\n\nbegin() and end() should be one-line too.\nCan we have begin_ and end_ to be iterator, so that begin() and end()\njust returns them, not dynamically construct iterator?\nFurthemore, can the variables and the function be constant?\n\n-Hiro\n\n> +private:\n> + Base begin_;\n> + Base end_;\n> +};\n> +\n> +} /* namespace details */\n> +\n> +template\n> +auto enumerate(T &iterable) -> details::enumerate_adapter\n> +{\n> + return { std::begin(iterable), std::end(iterable) };\n> +}\n> +\n> +#ifndef __DOXYGEN__\n> +template\n> +auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter\n> +{\n> + return { std::begin(iterable), std::end(iterable) };\n> +}\n> +#endif\n> +\n> } /* namespace utils */\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> index c4098a74e0ab..ff9a5832b10e 100644\n> --- a/src/libcamera/utils.cpp\n> +++ b/src/libcamera/utils.cpp\n> @@ -472,6 +472,35 @@ std::string libcameraSourcePath()\n> * loop, will cause the loop to iterate over the \\a iterable in reverse order\n> */\n>\n> +/**\n> + * \\fn enumerate(T &iterable)\n> + * \\brief Wrap an iterable to enumerate index and value in a range-based loop\n> + * \\param[in] iterable The iterable\n> + *\n> + * Range-based for loops are handy and widely preferred in C++, but are limited\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> + *\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> + *\n> + * \\code{.cpp}\n> + * std::vector values = ...;\n> + *\n> + * for (auto [index, value] : utils::enumerate(values)) {\n> + * ...\n> + * }\n> + * \\endcode\n> + *\n> + * \\return A value of unspecified type that, when used in a range-based for\n> + * loop, iterates over an indexed view of the \\a iterable\n> + */\n> +\n> } /* namespace utils */\n>\n> } /* namespace libcamera */\n> diff --git a/test/utils.cpp b/test/utils.cpp\n> index 08f293898fd9..7e24c71e4775 100644\n> --- a/test/utils.cpp\n> +++ b/test/utils.cpp\n> @@ -12,6 +12,7 @@\n> #include \n>\n> #include \n> +#include \n>\n> #include \"libcamera/internal/utils.h\"\n>\n> @@ -73,6 +74,60 @@ protected:\n> return TestPass;\n> }\n>\n> + int testEnumerate()\n> + {\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> + cerr << \"utils::enumerate() test failed: i=\" << i\n> + << \", index=\" << index << \", value=\" << value\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + /* Verify that we can modify the value. */\n> + --value;\n> + ++i;\n> + }\n> +\n> + if (integers != std::vector{ 0, 1, 2, 3, 4 }) {\n> + cerr << \"Failed to modify container in enumerated range loop\" << endl;\n> + return TestFail;\n> + }\n> +\n> + Span span{ integers };\n> + i = 0;\n> +\n> + for (auto [index, value] : utils::enumerate(span)) {\n> + if (index != i || value != i) {\n> + cerr << \"utils::enumerate() test failed: i=\" << i\n> + << \", index=\" << index << \", value=\" << value\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + ++i;\n> + }\n> +\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> + cerr << \"utils::enumerate() test failed: i=\" << i\n> + << \", index=\" << index << \", value=\" << value\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + ++i;\n> + }\n> +\n> + return TestPass;\n> + }\n> +\n> int run()\n> {\n> /* utils::hex() test. */\n> @@ -177,6 +232,10 @@ protected:\n> return TestFail;\n> }\n>\n> + /* utils::enumerate() test. */\n> + if (testEnumerate() != TestPass)\n> + return TestFail;\n> +\n> return TestPass;\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 238F9BDCA1\n\tfor ;\n\tTue, 27 Apr 2021 02:29:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5B3A068880;\n\tTue, 27 Apr 2021 04:29:55 +0200 (CEST)","from mail-ej1-x62f.google.com (mail-ej1-x62f.google.com\n\t[IPv6:2a00:1450:4864:20::62f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 68E72605BD\n\tfor ;\n\tTue, 27 Apr 2021 04:29:54 +0200 (CEST)","by mail-ej1-x62f.google.com with SMTP id u17so87444857ejk.2\n\tfor ;\n\tMon, 26 Apr 2021 19:29:54 -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=\"PRrmiT8e\"; 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=FSXlWypsIPGfGdiO5ww3jQmhGFVnJJsTGJjJsTo7Z6c=;\n\tb=PRrmiT8enebqNsX3rK7D2GG8fc6CMLGrLaiwAexzFZ8g/ZVO89VKIU4mIYX7ryPluc\n\t3qSJ2BWQ+Nmv6Zf1Em/ieNjTlbYHDTsBciBr6emCmfstXqIgOx8uKekEIRst/n25fGx2\n\tY7d/psgMm884Rs2D3ltIt8jkG2+1Z6J8paUE4=","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=FSXlWypsIPGfGdiO5ww3jQmhGFVnJJsTGJjJsTo7Z6c=;\n\tb=RruoL04QCvPpaOCXZ2L2MOmrVhl/LamwTWrknja04uTanyGemSCS7q/X6QKL7Rc9Ml\n\t7vI8gjwFbG8oznF3j4/99TyABmt3zDshjcHeaWXXGBDQSpvLfOjdqfDcuWHy8gAJuslq\n\tBDOQBbWymmgDQ5BGz1TBYOT2x1HN/B8hyOwbSK3+g0DJVuEkYjNtTl2jqi1D0MmKhbvT\n\t0w/YolvumS3U95jn5nt2Qz7UP9c9e4LRAMT92f4e2l0d8ozFiMAUa1tBd5fTfNGZeEBU\n\tP+7UAOPUeL24Ge1ZnAzaxHb9i31iTRhEG1xt1EwRDhvKXQhI24YA8tbjPP8eExaegk2q\n\trxig==","X-Gm-Message-State":"AOAM532Sahxu0c6P6D01dY9IsBm7F8V/ERJkswErODIyYOyJMQ/Z090+\n\tVEyGcOVBs0oVpOjE+maB8m7wpIPzxzgSiGrmhWbmNg==","X-Google-Smtp-Source":"ABdhPJweiSZ+vQVZ+0l7xZJUAvxmWF/BmOkVhQlcpRIDtfybdbI97lbIx2TXSU/+yYj0YBoE6hMyFwLTNYnRiY6Edts=","X-Received":"by 2002:a17:906:13d6:: with SMTP id\n\tg22mr21206265ejc.475.1619490593998; \n\tMon, 26 Apr 2021 19:29:53 -0700 (PDT)","MIME-Version":"1.0","References":"<20210423020932.2760-1-laurent.pinchart@ideasonboard.com>\n\t<20210423020932.2760-2-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20210423020932.2760-2-laurent.pinchart@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Tue, 27 Apr 2021 11:29:44 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH/RFC 1/3] libcamera: utils: Add\n\tenumerate view for range-based for loops","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":16892,"web_url":"https://patchwork.libcamera.org/comment/16892/","msgid":"<4e317e8e-5b4d-183c-f652-af689985c517@ideasonboard.com>","date":"2021-05-11T10:14:44","subject":"Re: [libcamera-devel] [PATCH/RFC 1/3] libcamera: utils: Add\n\tenumerate view for range-based for loops","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 03:29, Hirokazu Honda wrote:\n> Hi Laurent, thanks for the patch.\n> \n> On Fri, Apr 23, 2021 at 11:09 AM Laurent Pinchart\n> wrote:\n>>\n>> Range-based for loops are handy and widely preferred in C++, but are\n>> limited in their ability to replace for loops that require access to a\n>> loop counter. The enumerate() function solves this problem by wrapping\n>> the \\a iterable in an adapter that, when used as a range-expression,\n>> will provide iterators whose value_type is a pair of index and value\n>> reference.\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\n>> arrays.\n>>\n>> A typical usage pattern would use structured binding to store the index\n>> and value in two separate variables:\n>>\n>> std::vector values = ...;\n>>\n>> for (auto [index, value] : utils::enumerate(values)) {\n\nWell that answers my question in 3/3...\n\nWhen used like this, can index be passed by value, and value passed by\nreference?\n\nGiven that 'value' is actually supposed to be a reference, would that be\na better naming convention?\n\n>> ...\n>> }\n>>\n>> Signed-off-by: Laurent Pinchart \n>> ---\n>> include/libcamera/internal/utils.h | 86 ++++++++++++++++++++++++++++++\n>> src/libcamera/utils.cpp | 29 ++++++++++\n>> test/utils.cpp | 59 ++++++++++++++++++++\n>> 3 files changed, 174 insertions(+)\n>>\n>> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n>> index d0146b71727d..9372a75889ac 100644\n>> --- a/include/libcamera/internal/utils.h\n>> +++ b/include/libcamera/internal/utils.h\n>> @@ -9,12 +9,14 @@\n>>\n>> #include \n>> #include \n>> +#include \n>> #include \n>> #include \n>> #include \n>> #include \n>> #include \n>> #include \n>> +#include \n>> #include \n>>\n>> #ifndef __DOXYGEN__\n>> @@ -230,6 +232,90 @@ details::reverse_adapter reverse(T &&iterable)\n>> return { iterable };\n>> }\n>>\n>> +namespace details {\n>> +\n>> +template\n>> +class enumerate_iterator\n>> +{\n> \n> I always implemented my own iterator with deriving std::itrator.\n> Today I learned it is getting deprecated... :0\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>> + using pointer = value_type *;\n>> + using reference = value_type &;\n>> + using iterator_category = typename std::iterator_traits::iterator_category;\n> \n> This class implements only std::forward_iterator_tag. I think this\n> should be std::forward_iterator_tag.\n> \n>> +\n>> + explicit enumerate_iterator(Base iter)\n>> + : current_(iter), pos_(0)\n>> + {\n>> + }\n>> +\n>> + enumerate_iterator &operator++()\n>> + {\n>> + ++current_;\n>> + ++pos_;\n>> + return *this;\n>> + }\n>> +\n>> + bool operator!=(const enumerate_iterator &other) const\n>> + {\n>> + return current_ != other.current_;\n>> + }\n>> +\n>> + value_type operator*() const\n>> + {\n>> + return { pos_, *current_ };\n>> + }\n> \n> Write in one-line?\n> value_type operator*() const { return { pos_, *current_ }; }\n> \n>> +\n>> +private:\n>> + Base current_;\n>> + difference_type pos_;\n>> +};\n>> +\n>> +template\n>> +class enumerate_adapter\n>> +{\n>> +public:\n>> + using iterator = enumerate_iterator;\n>> +\n>> + enumerate_adapter(Base begin, Base end)\n>> + : begin_(begin), end_(end)\n>> + {\n>> + }\n>> +\n>> + iterator begin()\n>> + {\n>> + return iterator{ begin_ };\n>> + }\n>> +\n>> + iterator end()\n>> + {\n>> + return iterator{ end_ };\n>> + }\n>> +\n> \n> begin() and end() should be one-line too.\n> Can we have begin_ and end_ to be iterator, so that begin() and end()\n> just returns them, not dynamically construct iterator?\n> Furthemore, can the variables and the function be constant?\n> \n> -Hiro\n> \n>> +private:\n>> + Base begin_;\n>> + Base end_;\n>> +};\n>> +\n>> +} /* namespace details */\n>> +\n>> +template\n>> +auto enumerate(T &iterable) -> details::enumerate_adapter\n>> +{\n>> + return { std::begin(iterable), std::end(iterable) };\n>> +}\n>> +\n>> +#ifndef __DOXYGEN__\n>> +template\n>> +auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter\n>> +{\n>> + return { std::begin(iterable), std::end(iterable) };\n>> +}\n>> +#endif\n>> +\n>> } /* namespace utils */\n>>\n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n>> index c4098a74e0ab..ff9a5832b10e 100644\n>> --- a/src/libcamera/utils.cpp\n>> +++ b/src/libcamera/utils.cpp\n>> @@ -472,6 +472,35 @@ std::string libcameraSourcePath()\n>> * loop, will cause the loop to iterate over the \\a iterable in reverse order\n>> */\n>>\n>> +/**\n>> + * \\fn enumerate(T &iterable)\n>> + * \\brief Wrap an iterable to enumerate index and value in a range-based loop\n>> + * \\param[in] iterable The iterable\n>> + *\n>> + * Range-based for loops are handy and widely preferred in C++, but are limited\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>> + *\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>> + *\n>> + * \\code{.cpp}\n>> + * std::vector values = ...;\n>> + *\n>> + * for (auto [index, value] : utils::enumerate(values)) {\n>> + * ...\n>> + * }\n>> + * \\endcode\n>> + *\n>> + * \\return A value of unspecified type that, when used in a range-based for\n>> + * loop, iterates over an indexed view of the \\a iterable\n>> + */\n>> +\n>> } /* namespace utils */\n>>\n>> } /* namespace libcamera */\n>> diff --git a/test/utils.cpp b/test/utils.cpp\n>> index 08f293898fd9..7e24c71e4775 100644\n>> --- a/test/utils.cpp\n>> +++ b/test/utils.cpp\n>> @@ -12,6 +12,7 @@\n>> #include \n>>\n>> #include \n>> +#include \n>>\n>> #include \"libcamera/internal/utils.h\"\n>>\n>> @@ -73,6 +74,60 @@ protected:\n>> return TestPass;\n>> }\n>>\n>> + int testEnumerate()\n>> + {\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>> + cerr << \"utils::enumerate() test failed: i=\" << i\n>> + << \", index=\" << index << \", value=\" << value\n>> + << std::endl;\n>> + return TestFail;\n>> + }\n>> +\n>> + /* Verify that we can modify the value. */\n>> + --value;\n>> + ++i;\n>> + }\n>> +\n>> + if (integers != std::vector{ 0, 1, 2, 3, 4 }) {\n>> + cerr << \"Failed to modify container in enumerated range loop\" << endl;\n>> + return TestFail;\n>> + }\n>> +\n>> + Span span{ integers };\n>> + i = 0;\n>> +\n>> + for (auto [index, value] : utils::enumerate(span)) {\n>> + if (index != i || value != i) {\n>> + cerr << \"utils::enumerate() test failed: i=\" << i\n>> + << \", index=\" << index << \", value=\" << value\n>> + << std::endl;\n>> + return TestFail;\n>> + }\n>> +\n>> + ++i;\n>> + }\n>> +\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>> + cerr << \"utils::enumerate() test failed: i=\" << i\n>> + << \", index=\" << index << \", value=\" << value\n>> + << std::endl;\n>> + return TestFail;\n>> + }\n>> +\n>> + ++i;\n>> + }\n>> +\n\nIs there value in adding a test to ensure that if the user increments or\notherwise modifies index it will not affect the iteration?\n\nOr is index const already?\n\n\n>> + return TestPass;\n>> + }\n>> +\n>> int run()\n>> {\n>> /* utils::hex() test. */\n>> @@ -177,6 +232,10 @@ protected:\n>> return TestFail;\n>> }\n>>\n>> + /* utils::enumerate() test. */\n>> + if (testEnumerate() != TestPass)\n>> + return TestFail;\n>> +\n>> return TestPass;\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\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 B9F9DBF839\n\tfor ;\n\tTue, 11 May 2021 10:14:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3C1986890C;\n\tTue, 11 May 2021 12:14:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1560A61538\n\tfor ;\n\tTue, 11 May 2021 12:14:51 +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 2750A4A2;\n\tTue, 11 May 2021 12:14:48 +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=\"Ah4o7unw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620728088;\n\tbh=CsWGp+/W1++ZnPRxa028l3mAa+ebAwvNSAi3aj13SVc=;\n\th=Reply-To:To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=Ah4o7unwbDzH9gs030SZ8WP1pgu7I6NhHD44GKcAyDW/j2tw/q+dCAom7ZlGDfZTX\n\tFfNPr+zDfDkfRFk25tzjp81Ze3LPabZkvyOUe54roQAiEDP+zhh2SqeWbL2GhJ42Ff\n\t5aDYI3ARlGqR2S8iICu6vSQyAv1X7UFzq9Eg9h/E=","To":"Hirokazu Honda ,\n\tLaurent Pinchart ","References":"<20210423020932.2760-1-laurent.pinchart@ideasonboard.com>\n\t<20210423020932.2760-2-laurent.pinchart@ideasonboard.com>\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<4e317e8e-5b4d-183c-f652-af689985c517@ideasonboard.com>","Date":"Tue, 11 May 2021 11:14:44 +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 1/3] libcamera: utils: Add\n\tenumerate view for range-based for loops","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":16898,"web_url":"https://patchwork.libcamera.org/comment/16898/","msgid":"","date":"2021-05-11T11:38:17","subject":"Re: [libcamera-devel] [PATCH/RFC 1/3] libcamera: utils: Add\n\tenumerate view for range-based for loops","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:30 +0300, Laurent Pinchart wrote:\n> Range-based for loops are handy and widely preferred in C++, but are\n> limited in their ability to replace for loops that require access to a\n> loop counter. The enumerate() function solves this problem by wrapping\n> the \\a iterable in an adapter that, when used as a range-expression,\n> will provide iterators whose value_type is a pair of index and value\n> reference.\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\n> arrays.\n> \n> A typical usage pattern would use structured binding to store the index\n> and value in two separate variables:\n> \n> std::vector values = ...;\n> \n> for (auto [index, value] : utils::enumerate(values)) {\n> ...\n> }\n> \n> Signed-off-by: Laurent Pinchart \n\nThis is either really neat or I have written too much python lately :-)\n\nReviewed-by: Niklas Söderlund \n\n> ---\n> include/libcamera/internal/utils.h | 86 ++++++++++++++++++++++++++++++\n> src/libcamera/utils.cpp | 29 ++++++++++\n> test/utils.cpp | 59 ++++++++++++++++++++\n> 3 files changed, 174 insertions(+)\n> \n> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n> index d0146b71727d..9372a75889ac 100644\n> --- a/include/libcamera/internal/utils.h\n> +++ b/include/libcamera/internal/utils.h\n> @@ -9,12 +9,14 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> #include \n> #include \n> #include \n> +#include \n> #include \n> \n> #ifndef __DOXYGEN__\n> @@ -230,6 +232,90 @@ details::reverse_adapter reverse(T &&iterable)\n> \treturn { iterable };\n> }\n> \n> +namespace details {\n> +\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> +\tusing pointer = value_type *;\n> +\tusing reference = value_type &;\n> +\tusing iterator_category = typename std::iterator_traits::iterator_category;\n> +\n> +\texplicit enumerate_iterator(Base iter)\n> +\t\t: current_(iter), pos_(0)\n> +\t{\n> +\t}\n> +\n> +\tenumerate_iterator &operator++()\n> +\t{\n> +\t\t++current_;\n> +\t\t++pos_;\n> +\t\treturn *this;\n> +\t}\n> +\n> +\tbool operator!=(const enumerate_iterator &other) const\n> +\t{\n> +\t\treturn current_ != other.current_;\n> +\t}\n> +\n> +\tvalue_type operator*() const\n> +\t{\n> +\t\treturn { pos_, *current_ };\n> +\t}\n> +\n> +private:\n> +\tBase current_;\n> +\tdifference_type pos_;\n> +};\n> +\n> +template\n> +class enumerate_adapter\n> +{\n> +public:\n> +\tusing iterator = enumerate_iterator;\n> +\n> +\tenumerate_adapter(Base begin, Base end)\n> +\t\t: begin_(begin), end_(end)\n> +\t{\n> +\t}\n> +\n> +\titerator begin()\n> +\t{\n> +\t\treturn iterator{ begin_ };\n> +\t}\n> +\n> +\titerator end()\n> +\t{\n> +\t\treturn iterator{ end_ };\n> +\t}\n> +\n> +private:\n> +\tBase begin_;\n> +\tBase end_;\n> +};\n> +\n> +} /* namespace details */\n> +\n> +template\n> +auto enumerate(T &iterable) -> details::enumerate_adapter\n> +{\n> +\treturn { std::begin(iterable), std::end(iterable) };\n> +}\n> +\n> +#ifndef __DOXYGEN__\n> +template\n> +auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter\n> +{\n> +\treturn { std::begin(iterable), std::end(iterable) };\n> +}\n> +#endif\n> +\n> } /* namespace utils */\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> index c4098a74e0ab..ff9a5832b10e 100644\n> --- a/src/libcamera/utils.cpp\n> +++ b/src/libcamera/utils.cpp\n> @@ -472,6 +472,35 @@ std::string libcameraSourcePath()\n> * loop, will cause the loop to iterate over the \\a iterable in reverse order\n> */\n> \n> +/**\n> + * \\fn enumerate(T &iterable)\n> + * \\brief Wrap an iterable to enumerate index and value in a range-based loop\n> + * \\param[in] iterable The iterable\n> + *\n> + * Range-based for loops are handy and widely preferred in C++, but are limited\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> + *\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> + *\n> + * \\code{.cpp}\n> + * std::vector values = ...;\n> + *\n> + * for (auto [index, value] : utils::enumerate(values)) {\n> + * \t...\n> + * }\n> + * \\endcode\n> + *\n> + * \\return A value of unspecified type that, when used in a range-based for\n> + * loop, iterates over an indexed view of the \\a iterable\n> + */\n> +\n> } /* namespace utils */\n> \n> } /* namespace libcamera */\n> diff --git a/test/utils.cpp b/test/utils.cpp\n> index 08f293898fd9..7e24c71e4775 100644\n> --- a/test/utils.cpp\n> +++ b/test/utils.cpp\n> @@ -12,6 +12,7 @@\n> #include \n> \n> #include \n> +#include \n> \n> #include \"libcamera/internal/utils.h\"\n> \n> @@ -73,6 +74,60 @@ protected:\n> \t\treturn TestPass;\n> \t}\n> \n> +\tint testEnumerate()\n> +\t{\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\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\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++i;\n> +\t\t}\n> +\n> +\t\tif (integers != std::vector{ 0, 1, 2, 3, 4 }) {\n> +\t\t\tcerr << \"Failed to modify container in enumerated range loop\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\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\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\treturn TestFail;\n> +\t\t\t}\n> +\n> +\t\t\t++i;\n> +\t\t}\n> +\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\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\treturn TestFail;\n> +\t\t\t}\n> +\n> +\t\t\t++i;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> \tint run()\n> \t{\n> \t\t/* utils::hex() test. */\n> @@ -177,6 +232,10 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> +\t\t/* utils::enumerate() test. */\n> +\t\tif (testEnumerate() != TestPass)\n> +\t\t\treturn TestFail;\n> +\n> \t\treturn TestPass;\n> \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 92843BF839\n\tfor ;\n\tTue, 11 May 2021 11:38:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EADBB68918;\n\tTue, 11 May 2021 13:38:21 +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 67C3461538\n\tfor ;\n\tTue, 11 May 2021 13:38:20 +0200 (CEST)","by mail-lf1-x132.google.com with SMTP id c3so28155568lfs.7\n\tfor ;\n\tTue, 11 May 2021 04:38:20 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\th32sm2563409lfv.251.2021.05.11.04.38.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 11 May 2021 04:38:18 -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=\"JL5Z/JdE\"; 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=+sgGE9mbM+INk2e8eNYoDYqojmOTwtrqsfY3doguWzM=;\n\tb=JL5Z/JdE8U/eZKJfZfWUjpW3/g6lxpiIOQtQ8CFFATXGUx5ixIzoVhY1ARUmwlyFJa\n\tyslChPN9sq5bHNVN0rlyHOXYeO1J4ewWzaHBPPVrHkzkm2n6iZ8L3y4J8mQ53O2Ignj7\n\tA86x8g/KSJtAi6EfLyIIfQKD9n3XcANZvE3dAwg+FDORVCbJ1sUfEMqP1I2lweqfWQzI\n\tT4DFxOwPqu/uIuEe6JW+lxycmO0MgfGPB+s6qWU+KfWQn7NS++wjTj0EvEtYDaW8siYE\n\tkf7Cw/N2pXqWj3oNLYuI7dGxpkWvr3Er/aIAo/b22czfV5U1Bp57qiaORC7MfKo6ytVI\n\ttclw==","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=+sgGE9mbM+INk2e8eNYoDYqojmOTwtrqsfY3doguWzM=;\n\tb=l2DwHg/me035alFpCS1JVqtf3CqBax9VLmCaqfUnU7uSEpyB995KddB6f6JhDiMGjY\n\tucgON28071PrCt1155EVShU2OjYKh4ZL+FdznoNtfURGic0+7qL37RZnxlx0hAykMYxO\n\tHV7k/ENuBTrpDbHGgTA3TQuP5OmoiHJSYJkjPnbrFkhq/BRQuQzodCBS7Z6EFyjmYlNz\n\twRh058p5M/MNIemVc0AeR2hASgUw59nwx3EbZ4KjpVbvhVWSF85NNhz5PiWV4xP/HXXv\n\tgkmPfcO0t0Mt2cfnQ0xZlYapIRsLQvLBUpG+vsajRsVVgnRKrVIAT37FSc6vIMP0zNM/\n\theaw==","X-Gm-Message-State":"AOAM531wXpLb8Gi6ddo22nwIYoZFpDo6rUeVn1SLyB/QxzfYnI6UhNta\n\t/sGj8ugEGG5Ko1g5xSHv8GKAHw==","X-Google-Smtp-Source":"ABdhPJygkoEtOGdj9IqsO9QiIcD4xjb77YvakcuZ8ed6eRzyaa06XL80A3W8+c67+/+uLn+jSRpNyQ==","X-Received":"by 2002:ac2:4241:: with SMTP id\n\tm1mr19862077lfl.397.1620733099374; \n\tTue, 11 May 2021 04:38:19 -0700 (PDT)","Date":"Tue, 11 May 2021 13:38:17 +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-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210423020932.2760-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH/RFC 1/3] libcamera: utils: Add\n\tenumerate view for range-based for loops","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\" "}},{"id":16950,"web_url":"https://patchwork.libcamera.org/comment/16950/","msgid":"","date":"2021-05-15T02:47:45","subject":"Re: [libcamera-devel] [PATCH/RFC 1/3] libcamera: utils: Add\n\tenumerate view for range-based for loops","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, May 11, 2021 at 11:14:44AM +0100, Kieran Bingham wrote:\n> On 27/04/2021 03:29, Hirokazu Honda wrote:\n> > On Fri, Apr 23, 2021 at 11:09 AM Laurent Pinchart wrote:\n> >>\n> >> Range-based for loops are handy and widely preferred in C++, but are\n> >> limited in their ability to replace for loops that require access to a\n> >> loop counter. The enumerate() function solves this problem by wrapping\n> >> the \\a iterable in an adapter that, when used as a range-expression,\n> >> will provide iterators whose value_type is a pair of index and value\n> >> reference.\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\n> >> arrays.\n> >>\n> >> A typical usage pattern would use structured binding to store the index\n> >> and value in two separate variables:\n> >>\n> >> std::vector values = ...;\n> >>\n> >> for (auto [index, value] : utils::enumerate(values)) {\n> \n> Well that answers my question in 3/3...\n> \n> When used like this, can index be passed by value, and value passed by\n> reference?\n\nYes, that's what this patch implements, see\nenumerate_iterator::value_type returned by operator*() (note that\nthey're not passed, but returned). The index is actually a const value,\nto ensure that no code will modify it and assume the modification would\nhave a similar effect as modifying the loop counter in a regular for\nloop.\n\n> Given that 'value' is actually supposed to be a reference, would that be\n> a better naming convention?\n\nI'm not sure what you mean here. In any case, this would result in code\nsuch as\n\n\tauto cameras = cm_->cameras();\n\tfor (auto [index, camera] : utils::enumerate(cameras)) {\n\t\t...\n\t}\n\nso there's no variable named 'value'.\n\n> >> ...\n> >> }\n> >>\n> >> Signed-off-by: Laurent Pinchart \n> >> ---\n> >> include/libcamera/internal/utils.h | 86 ++++++++++++++++++++++++++++++\n> >> src/libcamera/utils.cpp | 29 ++++++++++\n> >> test/utils.cpp | 59 ++++++++++++++++++++\n> >> 3 files changed, 174 insertions(+)\n> >>\n> >> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n> >> index d0146b71727d..9372a75889ac 100644\n> >> --- a/include/libcamera/internal/utils.h\n> >> +++ b/include/libcamera/internal/utils.h\n> >> @@ -9,12 +9,14 @@\n> >>\n> >> #include \n> >> #include \n> >> +#include \n> >> #include \n> >> #include \n> >> #include \n> >> #include \n> >> #include \n> >> #include \n> >> +#include \n> >> #include \n> >>\n> >> #ifndef __DOXYGEN__\n> >> @@ -230,6 +232,90 @@ details::reverse_adapter reverse(T &&iterable)\n> >> return { iterable };\n> >> }\n> >>\n> >> +namespace details {\n> >> +\n> >> +template\n> >> +class enumerate_iterator\n> >> +{\n> > \n> > I always implemented my own iterator with deriving std::itrator.\n> > Today I learned it is getting deprecated... :0\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> >> + using pointer = value_type *;\n> >> + using reference = value_type &;\n> >> + using iterator_category = typename std::iterator_traits::iterator_category;\n> > \n> > This class implements only std::forward_iterator_tag. I think this\n> > should be std::forward_iterator_tag.\n> > \n> >> +\n> >> + explicit enumerate_iterator(Base iter)\n> >> + : current_(iter), pos_(0)\n> >> + {\n> >> + }\n> >> +\n> >> + enumerate_iterator &operator++()\n> >> + {\n> >> + ++current_;\n> >> + ++pos_;\n> >> + return *this;\n> >> + }\n> >> +\n> >> + bool operator!=(const enumerate_iterator &other) const\n> >> + {\n> >> + return current_ != other.current_;\n> >> + }\n> >> +\n> >> + value_type operator*() const\n> >> + {\n> >> + return { pos_, *current_ };\n> >> + }\n> > \n> > Write in one-line?\n> > value_type operator*() const { return { pos_, *current_ }; }\n> > \n> >> +\n> >> +private:\n> >> + Base current_;\n> >> + difference_type pos_;\n> >> +};\n> >> +\n> >> +template\n> >> +class enumerate_adapter\n> >> +{\n> >> +public:\n> >> + using iterator = enumerate_iterator;\n> >> +\n> >> + enumerate_adapter(Base begin, Base end)\n> >> + : begin_(begin), end_(end)\n> >> + {\n> >> + }\n> >> +\n> >> + iterator begin()\n> >> + {\n> >> + return iterator{ begin_ };\n> >> + }\n> >> +\n> >> + iterator end()\n> >> + {\n> >> + return iterator{ end_ };\n> >> + }\n> >> +\n> > \n> > begin() and end() should be one-line too.\n> > Can we have begin_ and end_ to be iterator, so that begin() and end()\n> > just returns them, not dynamically construct iterator?\n> > Furthemore, can the variables and the function be constant?\n> > \n> > -Hiro\n> > \n> >> +private:\n> >> + Base begin_;\n> >> + Base end_;\n> >> +};\n> >> +\n> >> +} /* namespace details */\n> >> +\n> >> +template\n> >> +auto enumerate(T &iterable) -> details::enumerate_adapter\n> >> +{\n> >> + return { std::begin(iterable), std::end(iterable) };\n> >> +}\n> >> +\n> >> +#ifndef __DOXYGEN__\n> >> +template\n> >> +auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter\n> >> +{\n> >> + return { std::begin(iterable), std::end(iterable) };\n> >> +}\n> >> +#endif\n> >> +\n> >> } /* namespace utils */\n> >>\n> >> } /* namespace libcamera */\n> >> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> >> index c4098a74e0ab..ff9a5832b10e 100644\n> >> --- a/src/libcamera/utils.cpp\n> >> +++ b/src/libcamera/utils.cpp\n> >> @@ -472,6 +472,35 @@ std::string libcameraSourcePath()\n> >> * loop, will cause the loop to iterate over the \\a iterable in reverse order\n> >> */\n> >>\n> >> +/**\n> >> + * \\fn enumerate(T &iterable)\n> >> + * \\brief Wrap an iterable to enumerate index and value in a range-based loop\n> >> + * \\param[in] iterable The iterable\n> >> + *\n> >> + * Range-based for loops are handy and widely preferred in C++, but are limited\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> >> + *\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> >> + *\n> >> + * \\code{.cpp}\n> >> + * std::vector values = ...;\n> >> + *\n> >> + * for (auto [index, value] : utils::enumerate(values)) {\n> >> + * ...\n> >> + * }\n> >> + * \\endcode\n> >> + *\n> >> + * \\return A value of unspecified type that, when used in a range-based for\n> >> + * loop, iterates over an indexed view of the \\a iterable\n> >> + */\n> >> +\n> >> } /* namespace utils */\n> >>\n> >> } /* namespace libcamera */\n> >> diff --git a/test/utils.cpp b/test/utils.cpp\n> >> index 08f293898fd9..7e24c71e4775 100644\n> >> --- a/test/utils.cpp\n> >> +++ b/test/utils.cpp\n> >> @@ -12,6 +12,7 @@\n> >> #include \n> >>\n> >> #include \n> >> +#include \n> >>\n> >> #include \"libcamera/internal/utils.h\"\n> >>\n> >> @@ -73,6 +74,60 @@ protected:\n> >> return TestPass;\n> >> }\n> >>\n> >> + int testEnumerate()\n> >> + {\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> >> + cerr << \"utils::enumerate() test failed: i=\" << i\n> >> + << \", index=\" << index << \", value=\" << value\n> >> + << std::endl;\n> >> + return TestFail;\n> >> + }\n> >> +\n> >> + /* Verify that we can modify the value. */\n> >> + --value;\n> >> + ++i;\n> >> + }\n> >> +\n> >> + if (integers != std::vector{ 0, 1, 2, 3, 4 }) {\n> >> + cerr << \"Failed to modify container in enumerated range loop\" << endl;\n> >> + return TestFail;\n> >> + }\n> >> +\n> >> + Span span{ integers };\n> >> + i = 0;\n> >> +\n> >> + for (auto [index, value] : utils::enumerate(span)) {\n> >> + if (index != i || value != i) {\n> >> + cerr << \"utils::enumerate() test failed: i=\" << i\n> >> + << \", index=\" << index << \", value=\" << value\n> >> + << std::endl;\n> >> + return TestFail;\n> >> + }\n> >> +\n> >> + ++i;\n> >> + }\n> >> +\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> >> + cerr << \"utils::enumerate() test failed: i=\" << i\n> >> + << \", index=\" << index << \", value=\" << value\n> >> + << std::endl;\n> >> + return TestFail;\n> >> + }\n> >> +\n> >> + ++i;\n> >> + }\n> >> +\n> \n> Is there value in adding a test to ensure that if the user increments or\n> otherwise modifies index it will not affect the iteration?\n> \n> Or is index const already?\n\nindex is const, so that test wouldn't compile :-)\n\n../../test/utils.cpp: In member function ‘int UtilsTest::testEnumerate()’:\n../../test/utils.cpp:93:25: error: increment of read-only reference ‘index’\n 93 | index++;\n | ^~~~~\n\n> >> + return TestPass;\n> >> + }\n> >> +\n> >> int run()\n> >> {\n> >> /* utils::hex() test. */\n> >> @@ -177,6 +232,10 @@ protected:\n> >> return TestFail;\n> >> }\n> >>\n> >> + /* utils::enumerate() test. */\n> >> + if (testEnumerate() != TestPass)\n> >> + return TestFail;\n> >> +\n> >> return TestPass;\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 7E6FAC31FA\n\tfor ;\n\tSat, 15 May 2021 02:47:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C937B68919;\n\tSat, 15 May 2021 04:47:58 +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 86D87602B3\n\tfor ;\n\tSat, 15 May 2021 04:47:56 +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 B5DC5559;\n\tSat, 15 May 2021 04:47:55 +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=\"ngyWsfq2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621046876;\n\tbh=J3ORKS4ydf5Acoqcd0PwH6hePHz2fp5BjoB8JiR+dAk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ngyWsfq2veQECxm589z7EyJLODuc274hmZvFOrnHB0KPTXK6SZYkg3ZQIJVRzQC5M\n\tnHeY7GiYdBL0dFQ3/RfHBMCZPNqmlRHQ4ys393FPIaGn3ATG6+7PxaHFq5WQaFrezV\n\tA8NdOPZBVSHzpRlnBkn7qtOvqVZTi8ClGYKwc/Gg=","Date":"Sat, 15 May 2021 05:47:45 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210423020932.2760-1-laurent.pinchart@ideasonboard.com>\n\t<20210423020932.2760-2-laurent.pinchart@ideasonboard.com>\n\t\n\t<4e317e8e-5b4d-183c-f652-af689985c517@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<4e317e8e-5b4d-183c-f652-af689985c517@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH/RFC 1/3] libcamera: utils: Add\n\tenumerate view for range-based for loops","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16952,"web_url":"https://patchwork.libcamera.org/comment/16952/","msgid":"","date":"2021-05-15T03:55:22","subject":"Re: [libcamera-devel] [PATCH/RFC 1/3] libcamera: utils: Add\n\tenumerate view for range-based for loops","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:29:44AM +0900, Hirokazu Honda wrote:\n> On Fri, Apr 23, 2021 at 11:09 AM Laurent Pinchart wrote:\n> >\n> > Range-based for loops are handy and widely preferred in C++, but are\n> > limited in their ability to replace for loops that require access to a\n> > loop counter. The enumerate() function solves this problem by wrapping\n> > the \\a iterable in an adapter that, when used as a range-expression,\n> > will provide iterators whose value_type is a pair of index and value\n> > reference.\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\n> > arrays.\n> >\n> > A typical usage pattern would use structured binding to store the index\n> > and value in two separate variables:\n> >\n> > std::vector values = ...;\n> >\n> > for (auto [index, value] : utils::enumerate(values)) {\n> > ...\n> > }\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/internal/utils.h | 86 ++++++++++++++++++++++++++++++\n> > src/libcamera/utils.cpp | 29 ++++++++++\n> > test/utils.cpp | 59 ++++++++++++++++++++\n> > 3 files changed, 174 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n> > index d0146b71727d..9372a75889ac 100644\n> > --- a/include/libcamera/internal/utils.h\n> > +++ b/include/libcamera/internal/utils.h\n> > @@ -9,12 +9,14 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > #ifndef __DOXYGEN__\n> > @@ -230,6 +232,90 @@ details::reverse_adapter reverse(T &&iterable)\n> > return { iterable };\n> > }\n> >\n> > +namespace details {\n> > +\n> > +template\n> > +class enumerate_iterator\n> > +{\n> \n> I always implemented my own iterator with deriving std::itrator.\n> Today I learned it is getting deprecated... :0\n\nI've learnt that quite recently too.\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> > + using pointer = value_type *;\n> > + using reference = value_type &;\n> > + using iterator_category = typename std::iterator_traits::iterator_category;\n> \n> This class implements only std::forward_iterator_tag. I think this\n> should be std::forward_iterator_tag.\n\nGood point. I think std::input_iterator_tag would be more appropriate,\nas the implementation doesn't fulfill the multipass guarantee defined in\nhttps://en.cppreference.com/w/cpp/named_req/ForwardIterator.\n\nTechnically speaking this isn't even a LegacyInputIterator, as it\ndoesn't implement the postfix ++ operator or the -> operator for\ninstance. Doing so may be possible (-> would be tricky as the value_type\nis currently constructed on-demand), but that wouldn't bring much added\nvalue. Still, std::input_iterator_tag is the best match for the intent.\n\n> > +\n> > + explicit enumerate_iterator(Base iter)\n> > + : current_(iter), pos_(0)\n> > + {\n> > + }\n> > +\n> > + enumerate_iterator &operator++()\n> > + {\n> > + ++current_;\n> > + ++pos_;\n> > + return *this;\n> > + }\n> > +\n> > + bool operator!=(const enumerate_iterator &other) const\n> > + {\n> > + return current_ != other.current_;\n> > + }\n> > +\n> > + value_type operator*() const\n> > + {\n> > + return { pos_, *current_ };\n> > + }\n> \n> Write in one-line?\n> value_type operator*() const { return { pos_, *current_ }; }\n\nWhen defining multiple inline functions, with some of them being\nmulti-line, I find it more readable to be consistent and define them all\non multiple lines.\n\n> > +\n> > +private:\n> > + Base current_;\n> > + difference_type pos_;\n> > +};\n> > +\n> > +template\n> > +class enumerate_adapter\n> > +{\n> > +public:\n> > + using iterator = enumerate_iterator;\n> > +\n> > + enumerate_adapter(Base begin, Base end)\n> > + : begin_(begin), end_(end)\n> > + {\n> > + }\n> > +\n> > + iterator begin()\n> > + {\n> > + return iterator{ begin_ };\n> > + }\n> > +\n> > + iterator end()\n> > + {\n> > + return iterator{ end_ };\n> > + }\n> > +\n> \n> begin() and end() should be one-line too.\n> Can we have begin_ and end_ to be iterator, so that begin() and end()\n> just returns them, not dynamically construct iterator?\n\nIt's doable (and fairly simple), but as we would have to return copies\nanyway, does it make much of a difference ?\n\n> Furthemore, can the variables and the function be constant?\n\nGood idea, will be done in v2.\n\n> > +private:\n> > + Base begin_;\n> > + Base end_;\n> > +};\n> > +\n> > +} /* namespace details */\n> > +\n> > +template\n> > +auto enumerate(T &iterable) -> details::enumerate_adapter\n> > +{\n> > + return { std::begin(iterable), std::end(iterable) };\n> > +}\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template\n> > +auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter\n> > +{\n> > + return { std::begin(iterable), std::end(iterable) };\n> > +}\n> > +#endif\n> > +\n> > } /* namespace utils */\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > index c4098a74e0ab..ff9a5832b10e 100644\n> > --- a/src/libcamera/utils.cpp\n> > +++ b/src/libcamera/utils.cpp\n> > @@ -472,6 +472,35 @@ std::string libcameraSourcePath()\n> > * loop, will cause the loop to iterate over the \\a iterable in reverse order\n> > */\n> >\n> > +/**\n> > + * \\fn enumerate(T &iterable)\n> > + * \\brief Wrap an iterable to enumerate index and value in a range-based loop\n> > + * \\param[in] iterable The iterable\n> > + *\n> > + * Range-based for loops are handy and widely preferred in C++, but are limited\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> > + *\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> > + *\n> > + * \\code{.cpp}\n> > + * std::vector values = ...;\n> > + *\n> > + * for (auto [index, value] : utils::enumerate(values)) {\n> > + * ...\n> > + * }\n> > + * \\endcode\n> > + *\n> > + * \\return A value of unspecified type that, when used in a range-based for\n> > + * loop, iterates over an indexed view of the \\a iterable\n> > + */\n> > +\n> > } /* namespace utils */\n> >\n> > } /* namespace libcamera */\n> > diff --git a/test/utils.cpp b/test/utils.cpp\n> > index 08f293898fd9..7e24c71e4775 100644\n> > --- a/test/utils.cpp\n> > +++ b/test/utils.cpp\n> > @@ -12,6 +12,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> >\n> > #include \"libcamera/internal/utils.h\"\n> >\n> > @@ -73,6 +74,60 @@ protected:\n> > return TestPass;\n> > }\n> >\n> > + int testEnumerate()\n> > + {\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> > + cerr << \"utils::enumerate() test failed: i=\" << i\n> > + << \", index=\" << index << \", value=\" << value\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /* Verify that we can modify the value. */\n> > + --value;\n> > + ++i;\n> > + }\n> > +\n> > + if (integers != std::vector{ 0, 1, 2, 3, 4 }) {\n> > + cerr << \"Failed to modify container in enumerated range loop\" << endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + Span span{ integers };\n> > + i = 0;\n> > +\n> > + for (auto [index, value] : utils::enumerate(span)) {\n> > + if (index != i || value != i) {\n> > + cerr << \"utils::enumerate() test failed: i=\" << i\n> > + << \", index=\" << index << \", value=\" << value\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + ++i;\n> > + }\n> > +\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> > + cerr << \"utils::enumerate() test failed: i=\" << i\n> > + << \", index=\" << index << \", value=\" << value\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + ++i;\n> > + }\n> > +\n> > + return TestPass;\n> > + }\n> > +\n> > int run()\n> > {\n> > /* utils::hex() test. */\n> > @@ -177,6 +232,10 @@ protected:\n> > return TestFail;\n> > }\n> >\n> > + /* utils::enumerate() test. */\n> > + if (testEnumerate() != TestPass)\n> > + return TestFail;\n> > +\n> > return TestPass;\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 88EF4C31FA\n\tfor ;\n\tSat, 15 May 2021 03:55:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B13EC6891C;\n\tSat, 15 May 2021 05:55:34 +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 CC292602B3\n\tfor ;\n\tSat, 15 May 2021 05:55:32 +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 2A3B36EE;\n\tSat, 15 May 2021 05:55:32 +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=\"itScI9dq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621050932;\n\tbh=MPrcE3jZSQ7z56y53kjSbX4mHQhpJcowM9N7j934MI0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=itScI9dqAhR3UC3l94n8gCUv/IcVemeQ6PrEFciLc1zx62A0E5p3JrkMbHLwemXwX\n\t8x+2Oof2YcBdVUWgo3Tbu3UM8RY4dSYqQdGNthePVlTxek8Btm9OPWyDn95EDRm16j\n\tJWiTx3cRfkTfvkTYlS8aSG7Ud+0F1e0xTQJTskPw=","Date":"Sat, 15 May 2021 06:55:22 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210423020932.2760-1-laurent.pinchart@ideasonboard.com>\n\t<20210423020932.2760-2-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH/RFC 1/3] libcamera: utils: Add\n\tenumerate view for range-based for loops","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\" "}}]