[{"id":22553,"web_url":"https://patchwork.libcamera.org/comment/22553/","msgid":"<20220401142826.iqkwb3wmsyr5f7p5@uno.localdomain>","date":"2022-04-01T14:28:26","subject":"Re: [libcamera-devel] [PATCH 3/5] provide a default fixed-sized\n\tSpan constructor","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Christian\n\nOn Fri, Apr 01, 2022 at 01:06:14AM +0100, Christian Rauch via libcamera-devel wrote:\n> The new default constructor allows to construct a fixed-sized Span via the\n> default constructor of its stored data type.\n> This prevents the construction of empty fixed-sized Spans that cannot hold\n> any data.\n>\n> Signed-off-by: Christian Rauch \n> ---\n> include/libcamera/base/span.h | 5 +++++\n> include/libcamera/controls.h | 2 +-\n> test/span.cpp | 2 +-\n> 3 files changed, 7 insertions(+), 2 deletions(-)\n>\n> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h\n> index 88d2e3de..7a4806dc 100644\n> --- a/include/libcamera/base/span.h\n> +++ b/include/libcamera/base/span.h\n> @@ -112,6 +112,11 @@ public:\n> \t{\n> \t}\n>\n> +\tSpan()\n> +\t{\n> +\t\tSpan(std::array{});\n> +\t}\n> +\n\nThis constructor creates a span of 'extent' elements all of them\ninitialized to 0 then ?\n\nIf I remove it I get\n\n../include/libcamera/controls.h:380:34: error: no matching function for call to ‘libcamera::Span::Span()’\n 380 | return T{};\n\nCaused by\n\ttemplate\n\tT get(const Control &ctrl) const\n\t{\n\t\tconst ControlValue *val = find(ctrl.id());\n\t\tif (!val)\n\t\t\treturn T{}; <------\n\n\t\treturn val->get();\n\t}\n\nas now that extent is != 0 the expression doesn't bind anymore to the\nexisting constructor:\n\n\ttemplate>\n\tconstexpr Span() noexcept\n\t\t: data_(nullptr)\n\t{\n\t}\n\n(I don't fully get what is the use of Dependent in the above definition :)\n\nHowever I feel like creating a Span of Extent elements all zeroed is a\nbit hill-defined, considering it is only used to return an error\ncondition. In example, if I'm not mistaken a Span constructed with\nyour proposed constructor will\n\n\tconstexpr size_type size() const noexcept { return Extent; }\n\tconstexpr size_type size_bytes() const noexcept { return size() * sizeof(element_type); }\n\tconstexpr bool empty() const noexcept { return size() == 0; }\n\nbe !empty and of size == 2, which is misleading considering we really\nwant to return an empty Span in case of error and the caller should\nnot be mis-leaded thinking it is valid.\n\nI don't have any super smart proposal to offer, if not recording the\n'valid' or 'empty' state in a class member and use it in size() and\nempty() instead of relying on Extent which is defined at overload\nresolution time and cannot be assigned at run time.\n\n> \texplicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)\n> \t\t: data_(ptr)\n> \t{\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 665bcac1..de8a7770 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -167,7 +167,7 @@ public:\n>\n> \t\tusing V = typename T::value_type;\n> \t\tconst V *value = reinterpret_cast(data().data());\n> -\t\treturn { value, numElements_ };\n> +\t\treturn T{ value, numElements_ };\n\nthis applies to overload\n\n\ttemplate::value ||\n\t\t\t\t\t\t std::is_same>::value,\n\tT get() const\n\t{\n\t\tassert(type_ == details::control_type>::value);\n\t\tassert(isArray_);\n\n\t\tusing V = typename T::value_type;\n\t\tconst V *value = reinterpret_cast(data().data());\n\t\treturn { value, numElements_ };\n\t}\n\nIsn't the returned { value, numElements_ } already used to construct an\ninstance of T if assigned to an lvalue expression ?\n\nIOW why do you need to contruct an instance of T and return it\nexplicitely ? Thanks to return value optimization this shouldn't have\nany performance impact but I have missed why it is needed.\n\n<2 minutes later>\n\nAnd now that I wrote this, if I remove it I get\n\ninclude/libcamera/controls.h:170:46: error: converting to\n‘libcamera::Span’ from initializer list would use\nexplicit constructor ‘constexpr libcamera::Span::Span(libcamera::Span::pointer, libcamera::Span::size_type) [with T = const float; long unsigned int Extent =\n2; libcamera::Span::pointer = const float*;\nlibcamera::Span::size_type = long unsigned int]’\n\nBecause indeed\n explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)\n\nis, well, explicit (which I think it's ok and should not be removed)\n\nI'll leave here the above text anyway for others that might have the\nsame question :)\n\n> \t}\n>\n> #ifndef __DOXYGEN__\n> diff --git a/test/span.cpp b/test/span.cpp\n> index abf3a5d6..c37e2a66 100644\n> --- a/test/span.cpp\n> +++ b/test/span.cpp\n> @@ -37,7 +37,7 @@ protected:\n> \t\t * to generate undefined behaviour.\n> \t\t */\n>\n> -\t\tSpan{};\n> +\t\t/* Span{}; */\n\nYou should remove it, or if the above issue with constructing an\n'empty' Span of size > 0 is resolved, prove that it works as intended.\n\n Span s{}\n if (!s.empty())\n error!\n\nThanks\n j\n\n> \t\t/* Span{}; */\n>\n> \t\tSpan{ &i[0], 4 };\n> --\n> 2.25.1\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 BB8D6C3256\n\tfor ;\n\tFri, 1 Apr 2022 14:28:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 03FF265633;\n\tFri, 1 Apr 2022 16:28:29 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D411633A6\n\tfor ;\n\tFri, 1 Apr 2022 16:28:28 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 0D84F20011;\n\tFri, 1 Apr 2022 14:28:27 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648823309;\n\tbh=4I2sK6C18bUEBiUxwSRtoTFxeIp7BUEGach3nAriwZ0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=hSQIuBNBNR3qfCWYvNc1V9yXIy39W6SN6rMensOvQtvlSrm99+YB+bUBxYNIwzz6V\n\tLSyHUAjq667fU8KtrzCoBqiEJbSAreClEwLtqMJ2wtkje7z4sWpOIxWqmbkGpEKxXS\n\t5+8ud+p4gVLBGAhJ+5KuX9alH/Cnua8nj9wqP74WLQjmbmjwUnyHYJYa5ao6Xas3tt\n\tQ461iCEqiQnz+7laoK90QhCBDymkODdmrKC0JYPulozJQFGwOK+LEoeaiVlkPdNj9t\n\tMVXUhiDqZj4jCKXAP4fEFw8rG8Xp5LqKxb/kQgIE0kLtd2JWtHGaRIzr5cCV2VLNwP\n\tN3TxixqKFHk9Q==","Date":"Fri, 1 Apr 2022 16:28:26 +0200","To":"Christian Rauch ","Message-ID":"<20220401142826.iqkwb3wmsyr5f7p5@uno.localdomain>","References":"<20220401000616.12976-1-Rauch.Christian@gmx.de>\n\t<20220401000616.12976-4-Rauch.Christian@gmx.de>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20220401000616.12976-4-Rauch.Christian@gmx.de>","Subject":"Re: [libcamera-devel] [PATCH 3/5] provide a default fixed-sized\n\tSpan constructor","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22554,"web_url":"https://patchwork.libcamera.org/comment/22554/","msgid":"<05bd0de4-5ee8-b56c-0f01-8b2115a6afd6@gmx.de>","date":"2022-04-01T23:05:43","subject":"Re: [libcamera-devel] [PATCH 3/5] provide a default fixed-sized\n\tSpan constructor","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Hi Jacopo,\n\nThanks for reviewing this. See my responses inline below.\n\n\nAm 01.04.22 um 15:28 schrieb Jacopo Mondi:\n> Hi Christian\n>\n> On Fri, Apr 01, 2022 at 01:06:14AM +0100, Christian Rauch via libcamera-devel wrote:\n>> The new default constructor allows to construct a fixed-sized Span via the\n>> default constructor of its stored data type.\n>> This prevents the construction of empty fixed-sized Spans that cannot hold\n>> any data.\n>>\n>> Signed-off-by: Christian Rauch \n>> ---\n>> include/libcamera/base/span.h | 5 +++++\n>> include/libcamera/controls.h | 2 +-\n>> test/span.cpp | 2 +-\n>> 3 files changed, 7 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h\n>> index 88d2e3de..7a4806dc 100644\n>> --- a/include/libcamera/base/span.h\n>> +++ b/include/libcamera/base/span.h\n>> @@ -112,6 +112,11 @@ public:\n>> \t{\n>> \t}\n>>\n>> +\tSpan()\n>> +\t{\n>> +\t\tSpan(std::array{});\n>> +\t}\n>> +\n>\n> This constructor creates a span of 'extent' elements all of them\n> initialized to 0 then ?\n\nAll elements will be default constructed. Integers and floats will be\ninitialised to 0, \"std::strings\" will be empty, and 'Size' and\n'Rectangle' will use their specified default constructors.\nThe default initialisation is required because the fixed-sized array\ncannot be empty.\n\n>\n> If I remove it I get\n>\n> ../include/libcamera/controls.h:380:34: error: no matching function for call to ‘libcamera::Span::Span()’\n> 380 | return T{};\n>\n> Caused by\n> \ttemplate\n> \tT get(const Control &ctrl) const\n> \t{\n> \t\tconst ControlValue *val = find(ctrl.id());\n> \t\tif (!val)\n> \t\t\treturn T{}; <------\n>\n> \t\treturn val->get();\n> \t}\n>\n> as now that extent is != 0 the expression doesn't bind anymore to the\n> existing constructor:\n>\n> \ttemplate \t\t typename = std::enable_if_t>\n> \tconstexpr Span() noexcept\n> \t\t: data_(nullptr)\n> \t{\n> \t}\n>\n> (I don't fully get what is the use of Dependent in the above definition :)\n\nI also do not understand the use of this constructor, since a\n0-fixed-size Span cannot store anything.\n>\n> However I feel like creating a Span of Extent elements all zeroed is a\n> bit hill-defined, considering it is only used to return an error\n> condition. In example, if I'm not mistaken a Span constructed with\n> your proposed constructor will\n>\n> \tconstexpr size_type size() const noexcept { return Extent; }\n> \tconstexpr size_type size_bytes() const noexcept { return size() * sizeof(element_type); }\n> \tconstexpr bool empty() const noexcept { return size() == 0; }\n>\n> be !empty and of size == 2, which is misleading considering we really\n> want to return an empty Span in case of error and the caller should\n> not be mis-leaded thinking it is valid.\n>\n> I don't have any super smart proposal to offer, if not recording the\n> 'valid' or 'empty' state in a class member and use it in size() and\n> empty() instead of relying on Extent which is defined at overload\n> resolution time and cannot be assigned at run time.\n\nI think this is a general issue with return default constructed objects\nto indicate an error. The error is only defined implicitly. If\nControlList::get returns a 'Size' with width=0 and height=0, then it is\nnot clear if this indicates an error or if this value is really stored.\n\nYou could add special class members to indicate that an object is\n\"valid\", but this is not a nice solution and only works for custom types\nin libcamera. The \"C\" way would probably to indicate an error by a NULL\npointer and a returned error number.\n\nI think the proper way would be to throw an exception. This could also\nbe a custom libcamera exception.\n\n>\n>> \texplicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)\n>> \t\t: data_(ptr)\n>> \t{\n>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>> index 665bcac1..de8a7770 100644\n>> --- a/include/libcamera/controls.h\n>> +++ b/include/libcamera/controls.h\n>> @@ -167,7 +167,7 @@ public:\n>>\n>> \t\tusing V = typename T::value_type;\n>> \t\tconst V *value = reinterpret_cast(data().data());\n>> -\t\treturn { value, numElements_ };\n>> +\t\treturn T{ value, numElements_ };\n>\n> this applies to overload\n>\n> \ttemplate::value ||\n> \t\t\t\t\t\t std::is_same>::value,\n> \tT get() const\n> \t{\n> \t\tassert(type_ == details::control_type>::value);\n> \t\tassert(isArray_);\n>\n> \t\tusing V = typename T::value_type;\n> \t\tconst V *value = reinterpret_cast(data().data());\n> \t\treturn { value, numElements_ };\n> \t}\n>\n> Isn't the returned { value, numElements_ } already used to construct an\n> instance of T if assigned to an lvalue expression ?\n>\n> IOW why do you need to contruct an instance of T and return it\n> explicitely ? Thanks to return value optimization this shouldn't have\n> any performance impact but I have missed why it is needed.\n>\n> <2 minutes later>\n>\n> And now that I wrote this, if I remove it I get\n>\n> include/libcamera/controls.h:170:46: error: converting to\n> ‘libcamera::Span’ from initializer list would use\n> explicit constructor ‘constexpr libcamera::Span Extent>::Span(libcamera::Span::pointer, libcamera::Span Extent>::size_type) [with T = const float; long unsigned int Extent =\n> 2; libcamera::Span::pointer = const float*;\n> libcamera::Span::size_type = long unsigned int]’\n>\n> Because indeed\n> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)\n>\n> is, well, explicit (which I think it's ok and should not be removed)\n>\n> I'll leave here the above text anyway for others that might have the\n> same question :)\n>\n>> \t}\n>>\n>> #ifndef __DOXYGEN__\n>> diff --git a/test/span.cpp b/test/span.cpp\n>> index abf3a5d6..c37e2a66 100644\n>> --- a/test/span.cpp\n>> +++ b/test/span.cpp\n>> @@ -37,7 +37,7 @@ protected:\n>> \t\t * to generate undefined behaviour.\n>> \t\t */\n>>\n>> -\t\tSpan{};\n>> +\t\t/* Span{}; */\n>\n> You should remove it, or if the above issue with constructing an\n> 'empty' Span of size > 0 is resolved, prove that it works as intended.\n\nI commented it out instead of removing this, because the comment above\nsays \"[...] Commented-out tests are expected not to compile [...]\" and\nthere was already a commented case for an 4-sized Span. Anyway, I don't\nsee a real application of a 0-fixed-sized Span, as it will never be able\nto store anything.\n\nBest,\nChristian\n\n\n>\n> Span s{}\n> if (!s.empty())\n> error!\n>\n> Thanks\n> j\n>\n>> \t\t/* Span{}; */\n>>\n>> \t\tSpan{ &i[0], 4 };\n>> --\n>> 2.25.1\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 0617BC0F1B\n\tfor ;\n\tFri, 1 Apr 2022 23:05:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 537C565633;\n\tSat, 2 Apr 2022 01:05:47 +0200 (CEST)","from mout.gmx.net (mout.gmx.net [212.227.17.21])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E84C6633A7\n\tfor ;\n\tSat, 2 Apr 2022 01:05:44 +0200 (CEST)","from [192.168.1.209] ([92.10.251.63]) by mail.gmx.net (mrgmx105\n\t[212.227.17.168]) with ESMTPSA (Nemesis) id 1MplXp-1oNZ0X0e8j-00q9TY;\n\tSat, 02 Apr 2022 01:05:44 +0200"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648854347;\n\tbh=SnZS2M3FC67HExoScvppJTMfNPLCFbllm6qPzcMTCnw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=mpv2x0QztGbB/MtVlKNB64vBU51Fn1fyigJixkopWX70ya4JoxR9Z4aPS0wHSxr2P\n\tynkO2y45JBXrpD5bZLzBimMqo/ZqQwzNmbjmzGhEzII0jKjWGlwpQxGxJ8HBrBbKqX\n\tknEFVswqXcCkQ0DLgrQH7n/GFjIxd9n5zN4To0ktkdaCQ41sXb3DAPf9X/c+BuFJ1w\n\t8dm7GdGz//LzMCPJ+OVdXmozVNEYbKotnA52d5Htau5/IwLvIqYHlE1G25XFB3Fzbz\n\tfdGNUOa6FJB0Lned4ntV4t501LMCv0mdAuCBi2dfHwcURmiNqKerMQxLzJVwobZWy+\n\tmGTnaImvDgs9w==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1648854344;\n\tbh=SnZS2M3FC67HExoScvppJTMfNPLCFbllm6qPzcMTCnw=;\n\th=X-UI-Sender-Class:Date:Subject:To:Cc:References:From:In-Reply-To;\n\tb=huwNYciP/aa2L5fVj5AmYrpp+q5eGU375gyHdKXTgncmO621xTbpKdyJ+OtDFN/rr\n\tdVIRyb0M+Ja1Lwkn8TkvnOT4h6dHS8oF2HV+IUD3uEBHq0HpC8rXdg2DekA7pEbZPS\n\teAd436M4U9Ok9zmIxrrtIDIx7jwiMNH6r4smF2aA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"huwNYciP\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"<05bd0de4-5ee8-b56c-0f01-8b2115a6afd6@gmx.de>","Date":"Sat, 2 Apr 2022 00:05:43 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.7.0","Content-Language":"en-GB","To":"Jacopo Mondi ","References":"<20220401000616.12976-1-Rauch.Christian@gmx.de>\n\t<20220401000616.12976-4-Rauch.Christian@gmx.de>\n\t<20220401142826.iqkwb3wmsyr5f7p5@uno.localdomain>","In-Reply-To":"<20220401142826.iqkwb3wmsyr5f7p5@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:X0xloveIboR/VRp822zOgja4wYzYksCtd3Hve1U4EApcN3kVfzC\n\t+ONIQEBDLOOAiQLKAJTKNbgjFbzwgLUlKtivB6hUNk3nRT+GO26V0G7qjVsGzzu9BjshBKD\n\t37H7dsoXZ6SOgPnSPtdTjGpDLIU3vALm+2uyl2NzbtdcTJJ3lo+Zri/EAFNFzHLe0nmOSXp\n\tfSvee2YsYUHjgwSSiksrA==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:dVfb4gILzx0=:8Zkns/RP9Qn1Q7+6jESEvl\n\tklcXF1qPfRBy6W9YVVuN1I4MHGZVYGfbZ9TC2s36HZEdjrXSXeisxCANkE/lu4BGnu0vVmjUq\n\tA6YO2CiG4ShskcR+wCOjmaLlyv4CBo6JvJKFazSg2T+sJfAa2sikxQGtDvhFQSIUw9ofUoKTQ\n\tlJPvAof65qPL29wCK78Ghed8tw4jkqCUTi0VXbIY+7B0h7Gw2l4tpRLtzaWej1Aq53OvGobsQ\n\t6ZGFDYp5pEa61ss8nF3fBS4604Ts6nNzHr4Wxk7Alup1boSyv2v4JOioV+N5mKqVyP9Helm1D\n\tgkG8ZcdOZaaZd1TSDpIrBB1w6AQSoRczeJ4dY2pluFPLhDlLpk/F5td8DZsjhKxgiORUnMWTp\n\tdvxllpW49ll1Da48hEKDqb3o6VcPMHxOCdQ+UzwXEi0NDGESDX1OKeMIazjevpMIdsMikvz86\n\tQ+zw+BTMEvVqotw83ef919K8sAK4EMBqtDEn6+6kKsprI5DMn8mRkLDJ3SkNZG7jkGh8wg1wB\n\t4lzGUTvzuHAliDTTfrNLdAFBFLMd3Pf5HWXY6YlD9dUHTJF9bIhFAjPM1mDxONKbS7gjTXWQP\n\tqkmSADjSKgGxDvRJ+Ev8lFHdA6jLykBSved10L3zen8enPCHVOq25KKSV/b3DTAGiA7I9XU+C\n\tVJKsWODTK4BK3Kl62voKZEPiac3jIPWcSdmW33ba2cBhQfVj4W7GOo+QlWHMFunPkKZHhrD+B\n\tbWjK8rm+LcrOhH51I9vBi5P0zmIz7Qp0yTodnpAsFkquv7BR4uSJaXVvQNb8GloFMFiefZuVC\n\tS8y9Rwve2oV6zPaKqTywikPUkyrELh9BShjP27cWF38syJMkqkf9tjI9+562uC8G+HTthxmNu\n\teDSQzbfKOx6WB9UY+b/Qq7OE9h7F608Sbq+ph7qSOOlZtQdenm3P64cwTEFQM/B6MRVl7uYwo\n\tdKD+3ZaX6F/1T0QCRtfZxNGkrqN5olTFdFprQFXkZgcTjN8qsmBYSGFwED4CKl79qvbuDP9tO\n\teUsAay3w0lV78xF/LaRVvT3SlyPr/QacypfVjeuDvlmWifIXVM3vF3ShTZzB9HcFCk2IktVGj\n\t4tulgruePk93Ug=","Subject":"Re: [libcamera-devel] [PATCH 3/5] provide a default fixed-sized\n\tSpan constructor","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","From":"Christian Rauch via libcamera-devel\n\t","Reply-To":"Christian Rauch ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22555,"web_url":"https://patchwork.libcamera.org/comment/22555/","msgid":"<20220402094614.y4slbqpyykm4j6te@uno.localdomain>","date":"2022-04-02T09:46:14","subject":"Re: [libcamera-devel] [PATCH 3/5] provide a default fixed-sized\n\tSpan constructor","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Christian,\n\nOn Sat, Apr 02, 2022 at 12:05:43AM +0100, Christian Rauch wrote:\n> Hi Jacopo,\n>\n> Thanks for reviewing this. See my responses inline below.\n>\n>\n> Am 01.04.22 um 15:28 schrieb Jacopo Mondi:\n> > Hi Christian\n> >\n> > On Fri, Apr 01, 2022 at 01:06:14AM +0100, Christian Rauch via libcamera-devel wrote:\n> >> The new default constructor allows to construct a fixed-sized Span via the\n> >> default constructor of its stored data type.\n> >> This prevents the construction of empty fixed-sized Spans that cannot hold\n> >> any data.\n> >>\n> >> Signed-off-by: Christian Rauch \n> >> ---\n> >> include/libcamera/base/span.h | 5 +++++\n> >> include/libcamera/controls.h | 2 +-\n> >> test/span.cpp | 2 +-\n> >> 3 files changed, 7 insertions(+), 2 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h\n> >> index 88d2e3de..7a4806dc 100644\n> >> --- a/include/libcamera/base/span.h\n> >> +++ b/include/libcamera/base/span.h\n> >> @@ -112,6 +112,11 @@ public:\n> >> \t{\n> >> \t}\n> >>\n> >> +\tSpan()\n> >> +\t{\n> >> +\t\tSpan(std::array{});\n> >> +\t}\n> >> +\n> >\n> > This constructor creates a span of 'extent' elements all of them\n> > initialized to 0 then ?\n>\n> All elements will be default constructed. Integers and floats will be\n> initialised to 0, \"std::strings\" will be empty, and 'Size' and\n> 'Rectangle' will use their specified default constructors.\n> The default initialisation is required because the fixed-sized array\n> cannot be empty.\n>\n\nYou're right, \"initialized to 0\" is not correct, \"default constructed\"\nis the term I should have used :)\n\n> >\n> > If I remove it I get\n> >\n> > ../include/libcamera/controls.h:380:34: error: no matching function for call to ‘libcamera::Span::Span()’\n> > 380 | return T{};\n> >\n> > Caused by\n> > \ttemplate\n> > \tT get(const Control &ctrl) const\n> > \t{\n> > \t\tconst ControlValue *val = find(ctrl.id());\n> > \t\tif (!val)\n> > \t\t\treturn T{}; <------\n> >\n> > \t\treturn val->get();\n> > \t}\n> >\n> > as now that extent is != 0 the expression doesn't bind anymore to the\n> > existing constructor:\n> >\n> > \ttemplate > \t\t typename = std::enable_if_t>\n> > \tconstexpr Span() noexcept\n> > \t\t: data_(nullptr)\n> > \t{\n> > \t}\n> >\n> > (I don't fully get what is the use of Dependent in the above definition :)\n>\n> I also do not understand the use of this constructor, since a\n> 0-fixed-size Span cannot store anything.\n\nI think it was used to identify a non-valid span, to be returned in\ncase of errors\n\n> >\n> > However I feel like creating a Span of Extent elements all zeroed is a\n> > bit hill-defined, considering it is only used to return an error\n> > condition. In example, if I'm not mistaken a Span constructed with\n> > your proposed constructor will\n> >\n> > \tconstexpr size_type size() const noexcept { return Extent; }\n> > \tconstexpr size_type size_bytes() const noexcept { return size() * sizeof(element_type); }\n> > \tconstexpr bool empty() const noexcept { return size() == 0; }\n> >\n> > be !empty and of size == 2, which is misleading considering we really\n> > want to return an empty Span in case of error and the caller should\n> > not be mis-leaded thinking it is valid.\n> >\n> > I don't have any super smart proposal to offer, if not recording the\n> > 'valid' or 'empty' state in a class member and use it in size() and\n> > empty() instead of relying on Extent which is defined at overload\n> > resolution time and cannot be assigned at run time.\n>\n> I think this is a general issue with return default constructed objects\n> to indicate an error. The error is only defined implicitly. If\n> ControlList::get returns a 'Size' with width=0 and height=0, then it is\n> not clear if this indicates an error or if this value is really stored.\n\nYes and no, if the custom type has a well defined interface it can be\nprovided with an isValid()/isNull() function, like it is done for the\nSize class\n\n\tbool isNull() const { return !width && !height; }\n\nI understand that\n\n Size s = controlList.get(SomeSizeControl);\n if (s.isNull()) {\n /* Error out */\n }\n\nRequires knowledge of the custom type interface and special care as\nit's less intuitive than handling an int return code or a pointer, but\neither we forbid return-by-value function signatures completely or we\nhave to instrument custom types with an isValid() function for the\ncaller to test.\n\nThe Span class validity test is based on its Extent size, and the\nconstructor you have introduced makes an invalid Span look like it is\nvalid.\n\n>\n> You could add special class members to indicate that an object is\n> \"valid\", but this is not a nice solution and only works for custom types\n\nOther custom libcamera types are fine in that regard. I was suggesting\nto add a 'bool valid_;' class member to Span, to not rely on Extent to\ntest its validity.\n\n> in libcamera. The \"C\" way would probably to indicate an error by a NULL\n> pointer and a returned error number.\n>\n> I think the proper way would be to throw an exception. This could also\n> be a custom libcamera exception.\n>\n\nToo bad libcamera doesn't use exceptions ;)\n\n> >\n> >> \texplicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)\n> >> \t\t: data_(ptr)\n> >> \t{\n> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >> index 665bcac1..de8a7770 100644\n> >> --- a/include/libcamera/controls.h\n> >> +++ b/include/libcamera/controls.h\n> >> @@ -167,7 +167,7 @@ public:\n> >>\n> >> \t\tusing V = typename T::value_type;\n> >> \t\tconst V *value = reinterpret_cast(data().data());\n> >> -\t\treturn { value, numElements_ };\n> >> +\t\treturn T{ value, numElements_ };\n> >\n> > this applies to overload\n> >\n> > \ttemplate::value ||\n> > \t\t\t\t\t\t std::is_same>::value,\n> > \tT get() const\n> > \t{\n> > \t\tassert(type_ == details::control_type>::value);\n> > \t\tassert(isArray_);\n> >\n> > \t\tusing V = typename T::value_type;\n> > \t\tconst V *value = reinterpret_cast(data().data());\n> > \t\treturn { value, numElements_ };\n> > \t}\n> >\n> > Isn't the returned { value, numElements_ } already used to construct an\n> > instance of T if assigned to an lvalue expression ?\n> >\n> > IOW why do you need to contruct an instance of T and return it\n> > explicitely ? Thanks to return value optimization this shouldn't have\n> > any performance impact but I have missed why it is needed.\n> >\n> > <2 minutes later>\n> >\n> > And now that I wrote this, if I remove it I get\n> >\n> > include/libcamera/controls.h:170:46: error: converting to\n> > ‘libcamera::Span’ from initializer list would use\n> > explicit constructor ‘constexpr libcamera::Span > Extent>::Span(libcamera::Span::pointer, libcamera::Span > Extent>::size_type) [with T = const float; long unsigned int Extent =\n> > 2; libcamera::Span::pointer = const float*;\n> > libcamera::Span::size_type = long unsigned int]’\n> >\n> > Because indeed\n> > explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)\n> >\n> > is, well, explicit (which I think it's ok and should not be removed)\n> >\n> > I'll leave here the above text anyway for others that might have the\n> > same question :)\n> >\n> >> \t}\n> >>\n> >> #ifndef __DOXYGEN__\n> >> diff --git a/test/span.cpp b/test/span.cpp\n> >> index abf3a5d6..c37e2a66 100644\n> >> --- a/test/span.cpp\n> >> +++ b/test/span.cpp\n> >> @@ -37,7 +37,7 @@ protected:\n> >> \t\t * to generate undefined behaviour.\n> >> \t\t */\n> >>\n> >> -\t\tSpan{};\n> >> +\t\t/* Span{}; */\n> >\n> > You should remove it, or if the above issue with constructing an\n> > 'empty' Span of size > 0 is resolved, prove that it works as intended.\n>\n> I commented it out instead of removing this, because the comment above\n> says \"[...] Commented-out tests are expected not to compile [...]\" and\n> there was already a commented case for an 4-sized Span. Anyway, I don't\n\nOh I see, sorry, missed it.\n\n> see a real application of a 0-fixed-sized Span, as it will never be able\n> to store anything.\n\nMy understanding is that it is only used for error conditions.\n\nThanks\n j\n\n>\n> Best,\n> Christian\n>\n>\n> >\n> > Span s{}\n> > if (!s.empty())\n> > error!\n> >\n> > Thanks\n> > j\n> >\n> >> \t\t/* Span{}; */\n> >>\n> >> \t\tSpan{ &i[0], 4 };\n> >> --\n> >> 2.25.1\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 1BA21C3256\n\tfor ;\n\tSat, 2 Apr 2022 09:46:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6053865644;\n\tSat, 2 Apr 2022 11:46:19 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1E355633A4\n\tfor ;\n\tSat, 2 Apr 2022 11:46:17 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 7E203240004;\n\tSat, 2 Apr 2022 09:46:16 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648892779;\n\tbh=FD0wiEXMV+67Z6UiSXza02FgbA/nQd940Omwf265OSM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=p8Z9dZKgkBpN87rci5Nna39Rm2xbWDX5FLRU0xkU0FTWWTfCrskBOeD6SnbArx8f3\n\t1LUAxb7zsqFhY389F6NpzknTgIML0zJiOCSxIq3XJ1OAbqiTS6yezJ2Gdg5twX1sPe\n\tFCY/AkTdHGHDGqMBmHj50ZaE8Blgt7DIDOEIVbRShdAlVsnpHgxDhH0sz9zwfMdI4W\n\tWXzJs/apWv1h3gsykR19JDajh+1yHm0QIdKXd9F/TziuN4nqZuZWIFC5Rc87XfguMI\n\tYO4kNk5Laxw6JSS5vUC5/geqL6uWftJUmNSX8yGm8+3PmMbCtfTdK4lE9dtuW+4bWO\n\tEeQe65SyD4ZFA==","Date":"Sat, 2 Apr 2022 11:46:14 +0200","To":"Christian Rauch ","Message-ID":"<20220402094614.y4slbqpyykm4j6te@uno.localdomain>","References":"<20220401000616.12976-1-Rauch.Christian@gmx.de>\n\t<20220401000616.12976-4-Rauch.Christian@gmx.de>\n\t<20220401142826.iqkwb3wmsyr5f7p5@uno.localdomain>\n\t<05bd0de4-5ee8-b56c-0f01-8b2115a6afd6@gmx.de>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<05bd0de4-5ee8-b56c-0f01-8b2115a6afd6@gmx.de>","Subject":"Re: [libcamera-devel] [PATCH 3/5] provide a default fixed-sized\n\tSpan constructor","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22556,"web_url":"https://patchwork.libcamera.org/comment/22556/","msgid":"<33eec917-3c50-ba76-5060-cd62211846f3@gmx.de>","date":"2022-04-02T11:28:05","subject":"Re: [libcamera-devel] [PATCH 3/5] provide a default fixed-sized\n\tSpan constructor","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Hi Jacopo,\n\nSee my response below, specifically see the case about \"std::optional\".\n\nAm 02.04.22 um 10:46 schrieb Jacopo Mondi:\n> Hi Christian,\n>\n> On Sat, Apr 02, 2022 at 12:05:43AM +0100, Christian Rauch wrote:\n>> Hi Jacopo,\n>>\n>> Thanks for reviewing this. See my responses inline below.\n>>\n>>\n>> Am 01.04.22 um 15:28 schrieb Jacopo Mondi:\n>>> Hi Christian\n>>>\n>>> On Fri, Apr 01, 2022 at 01:06:14AM +0100, Christian Rauch via libcamera-devel wrote:\n>>>> The new default constructor allows to construct a fixed-sized Span via the\n>>>> default constructor of its stored data type.\n>>>> This prevents the construction of empty fixed-sized Spans that cannot hold\n>>>> any data.\n>>>>\n>>>> Signed-off-by: Christian Rauch \n>>>> ---\n>>>> include/libcamera/base/span.h | 5 +++++\n>>>> include/libcamera/controls.h | 2 +-\n>>>> test/span.cpp | 2 +-\n>>>> 3 files changed, 7 insertions(+), 2 deletions(-)\n>>>>\n>>>> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h\n>>>> index 88d2e3de..7a4806dc 100644\n>>>> --- a/include/libcamera/base/span.h\n>>>> +++ b/include/libcamera/base/span.h\n>>>> @@ -112,6 +112,11 @@ public:\n>>>> \t{\n>>>> \t}\n>>>>\n>>>> +\tSpan()\n>>>> +\t{\n>>>> +\t\tSpan(std::array{});\n>>>> +\t}\n>>>> +\n>>>\n>>> This constructor creates a span of 'extent' elements all of them\n>>> initialized to 0 then ?\n>>\n>> All elements will be default constructed. Integers and floats will be\n>> initialised to 0, \"std::strings\" will be empty, and 'Size' and\n>> 'Rectangle' will use their specified default constructors.\n>> The default initialisation is required because the fixed-sized array\n>> cannot be empty.\n>>\n>\n> You're right, \"initialized to 0\" is not correct, \"default constructed\"\n> is the term I should have used :)\n>\n>>>\n>>> If I remove it I get\n>>>\n>>> ../include/libcamera/controls.h:380:34: error: no matching function for call to ‘libcamera::Span::Span()’\n>>> 380 | return T{};\n>>>\n>>> Caused by\n>>> \ttemplate\n>>> \tT get(const Control &ctrl) const\n>>> \t{\n>>> \t\tconst ControlValue *val = find(ctrl.id());\n>>> \t\tif (!val)\n>>> \t\t\treturn T{}; <------\n>>>\n>>> \t\treturn val->get();\n>>> \t}\n>>>\n>>> as now that extent is != 0 the expression doesn't bind anymore to the\n>>> existing constructor:\n>>>\n>>> \ttemplate>> \t\t typename = std::enable_if_t>\n>>> \tconstexpr Span() noexcept\n>>> \t\t: data_(nullptr)\n>>> \t{\n>>> \t}\n>>>\n>>> (I don't fully get what is the use of Dependent in the above definition :)\n>>\n>> I also do not understand the use of this constructor, since a\n>> 0-fixed-size Span cannot store anything.\n>\n> I think it was used to identify a non-valid span, to be returned in\n> case of errors\n>\n>>>\n>>> However I feel like creating a Span of Extent elements all zeroed is a\n>>> bit hill-defined, considering it is only used to return an error\n>>> condition. In example, if I'm not mistaken a Span constructed with\n>>> your proposed constructor will\n>>>\n>>> \tconstexpr size_type size() const noexcept { return Extent; }\n>>> \tconstexpr size_type size_bytes() const noexcept { return size() * sizeof(element_type); }\n>>> \tconstexpr bool empty() const noexcept { return size() == 0; }\n>>>\n>>> be !empty and of size == 2, which is misleading considering we really\n>>> want to return an empty Span in case of error and the caller should\n>>> not be mis-leaded thinking it is valid.\n>>>\n>>> I don't have any super smart proposal to offer, if not recording the\n>>> 'valid' or 'empty' state in a class member and use it in size() and\n>>> empty() instead of relying on Extent which is defined at overload\n>>> resolution time and cannot be assigned at run time.\n>>\n>> I think this is a general issue with return default constructed objects\n>> to indicate an error. The error is only defined implicitly. If\n>> ControlList::get returns a 'Size' with width=0 and height=0, then it is\n>> not clear if this indicates an error or if this value is really stored.\n>\n> Yes and no, if the custom type has a well defined interface it can be\n> provided with an isValid()/isNull() function, like it is done for the\n> Size class\n>\n> \tbool isNull() const { return !width && !height; }\n>\n> I understand that\n>\n> Size s = controlList.get(SomeSizeControl);\n> if (s.isNull()) {\n> /* Error out */\n> }\n>\n> Requires knowledge of the custom type interface and special care as\n> it's less intuitive than handling an int return code or a pointer, but\n> either we forbid return-by-value function signatures completely or we\n> have to instrument custom types with an isValid() function for the\n> caller to test.\n\nBut this will only ever work for custom types. Basic C types and C++\nstandard types do not have such a flag. \"isNull\" only means that the\narea of the Size is zero, but it does not directly mean that the\nrequested \"Size\" does not exist.\n\n>\n> The Span class validity test is based on its Extent size, and the\n> constructor you have introduced makes an invalid Span look like it is\n> valid.\n\nI really don't like this idea of implicitly encoding \"validity\" by a\nspecific value :-)\n\n>\n>>\n>> You could add special class members to indicate that an object is\n>> \"valid\", but this is not a nice solution and only works for custom types\n>\n> Other custom libcamera types are fine in that regard. I was suggesting\n> to add a 'bool valid_;' class member to Span, to not rely on Extent to\n> test its validity.\n>\n>> in libcamera. The \"C\" way would probably to indicate an error by a NULL\n>> pointer and a returned error number.\n>>\n>> I think the proper way would be to throw an exception. This could also\n>> be a custom libcamera exception.\n>>\n>\n> Too bad libcamera doesn't use exceptions ;)\n\nWhy is that? Has this something to do with \"embedded C++\"? Alternatively\nto exceptions, the get method could either 1) take a target value by\nreference and return a status flag (success / failure) or 2) take a\nstatus flag by reference and return a default constructed object.\n\nThere is also \"std::optional<>\" which can express this: \"A common use\ncase for optional is the return value of a function that may fail.\"\n\nIf you are ok with newer C++ features, than \"optional\" is maybe the most\n\"expressive\" solution:\n\n std::optional ret = controlList.get(SomeSizeControl);\n if (ret.has_value()) {\n // OK\n Size s = s.value();\n }\n else {\n // ERROR\n }\n\nWhat do you think about this? The \"optional\" is essentially attaching a\nvalidity flag to any object returned by the \"get\" method, but it does\nnot require changing the type itself.\n\n>\n>>>\n>>>> \texplicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)\n>>>> \t\t: data_(ptr)\n>>>> \t{\n>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>>> index 665bcac1..de8a7770 100644\n>>>> --- a/include/libcamera/controls.h\n>>>> +++ b/include/libcamera/controls.h\n>>>> @@ -167,7 +167,7 @@ public:\n>>>>\n>>>> \t\tusing V = typename T::value_type;\n>>>> \t\tconst V *value = reinterpret_cast(data().data());\n>>>> -\t\treturn { value, numElements_ };\n>>>> +\t\treturn T{ value, numElements_ };\n>>>\n>>> this applies to overload\n>>>\n>>> \ttemplate::value ||\n>>> \t\t\t\t\t\t std::is_same>::value,\n>>> \tT get() const\n>>> \t{\n>>> \t\tassert(type_ == details::control_type>::value);\n>>> \t\tassert(isArray_);\n>>>\n>>> \t\tusing V = typename T::value_type;\n>>> \t\tconst V *value = reinterpret_cast(data().data());\n>>> \t\treturn { value, numElements_ };\n>>> \t}\n>>>\n>>> Isn't the returned { value, numElements_ } already used to construct an\n>>> instance of T if assigned to an lvalue expression ?\n>>>\n>>> IOW why do you need to contruct an instance of T and return it\n>>> explicitely ? Thanks to return value optimization this shouldn't have\n>>> any performance impact but I have missed why it is needed.\n>>>\n>>> <2 minutes later>\n>>>\n>>> And now that I wrote this, if I remove it I get\n>>>\n>>> include/libcamera/controls.h:170:46: error: converting to\n>>> ‘libcamera::Span’ from initializer list would use\n>>> explicit constructor ‘constexpr libcamera::Span>> Extent>::Span(libcamera::Span::pointer, libcamera::Span>> Extent>::size_type) [with T = const float; long unsigned int Extent =\n>>> 2; libcamera::Span::pointer = const float*;\n>>> libcamera::Span::size_type = long unsigned int]’\n>>>\n>>> Because indeed\n>>> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)\n>>>\n>>> is, well, explicit (which I think it's ok and should not be removed)\n>>>\n>>> I'll leave here the above text anyway for others that might have the\n>>> same question :)\n>>>\n>>>> \t}\n>>>>\n>>>> #ifndef __DOXYGEN__\n>>>> diff --git a/test/span.cpp b/test/span.cpp\n>>>> index abf3a5d6..c37e2a66 100644\n>>>> --- a/test/span.cpp\n>>>> +++ b/test/span.cpp\n>>>> @@ -37,7 +37,7 @@ protected:\n>>>> \t\t * to generate undefined behaviour.\n>>>> \t\t */\n>>>>\n>>>> -\t\tSpan{};\n>>>> +\t\t/* Span{}; */\n>>>\n>>> You should remove it, or if the above issue with constructing an\n>>> 'empty' Span of size > 0 is resolved, prove that it works as intended.\n>>\n>> I commented it out instead of removing this, because the comment above\n>> says \"[...] Commented-out tests are expected not to compile [...]\" and\n>> there was already a commented case for an 4-sized Span. Anyway, I don't\n>\n> Oh I see, sorry, missed it.\n>\n>> see a real application of a 0-fixed-sized Span, as it will never be able\n>> to store anything.\n>\n> My understanding is that it is only used for error conditions.\n\nIn this case, this \"error condition\" will be replaced by another\ncompiler error that will trigger for the same reasons (empty span), but\nwill be triggered by trying to construct an empty \"std::array\".\n\nBest,\nChristian\n\n>\n> Thanks\n> j\n>\n>>\n>> Best,\n>> Christian\n>>\n>>\n>>>\n>>> Span s{}\n>>> if (!s.empty())\n>>> error!\n>>>\n>>> Thanks\n>>> j\n>>>\n>>>> \t\t/* Span{}; */\n>>>>\n>>>> \t\tSpan{ &i[0], 4 };\n>>>> --\n>>>> 2.25.1\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 5725FC0F1B\n\tfor ;\n\tSat, 2 Apr 2022 11:28:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9305365640;\n\tSat, 2 Apr 2022 13:28:08 +0200 (CEST)","from mout.gmx.net (mout.gmx.net [212.227.15.15])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BCC13604BE\n\tfor ;\n\tSat, 2 Apr 2022 13:28:06 +0200 (CEST)","from [192.168.1.209] ([92.10.251.63]) by mail.gmx.net (mrgmx004\n\t[212.227.17.190]) with ESMTPSA (Nemesis) id 1M2O2W-1na4Ua10XO-003tu8;\n\tSat, 02 Apr 2022 13:28:06 +0200"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648898888;\n\tbh=5Dh7nsxfc0LwXXUS0F+8cFa3zdQPaasPhD1c9o+L+EU=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=f2CLsiRX9EHXwUsqU7md0rRNotGZUMw2Y3BmPZOUgGuQe+GQjidhZtdnaSR/NFlFq\n\tTwN3g1HCzN82wsSL0dY6ye5MJOtS0Kb80Cj9aO4a8zg8Y8M9S3s++xxUawS4uVOVTT\n\t5jPegGNS5vxa/qQhFsrRuSedJ8gY5jePQtCzrJkvIysicv9GOnpJlBH3lT2kTIvlsT\n\tAXkIeHzIttJiRnCTeN5vMwzP3C5+n5va3HOyF93wgVFIIV2tCtMYh7rcENXKDFwqKf\n\tpQWoAVb+9GttHZUodRyv/zHIpECMJwm9+41kwU1jxTaXVXNqMVqYAEKzy/Viisw1aV\n\tlNcqnEhp5vdbw==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1648898886;\n\tbh=5Dh7nsxfc0LwXXUS0F+8cFa3zdQPaasPhD1c9o+L+EU=;\n\th=X-UI-Sender-Class:Date:Subject:To:Cc:References:From:In-Reply-To;\n\tb=VNuyS9My+lv/elzQ9WPTcY2QbnfW4k9gSbH/AwaCjZRs8bwrpJ6iG7QF421QW+JKJ\n\tDWY2hdQmM10vV0OIbRfJMIAvxtFFGQEIjNMBbK+DjVDO38iEPiZxnKsfyxQ/FFeE7V\n\tUedNYJVLTkqZrXD0RN01R5LG2cgkW81zRoy6fX8o="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"VNuyS9My\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"<33eec917-3c50-ba76-5060-cd62211846f3@gmx.de>","Date":"Sat, 2 Apr 2022 12:28:05 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.7.0","Content-Language":"en-CA","To":"Jacopo Mondi ","References":"<20220401000616.12976-1-Rauch.Christian@gmx.de>\n\t<20220401000616.12976-4-Rauch.Christian@gmx.de>\n\t<20220401142826.iqkwb3wmsyr5f7p5@uno.localdomain>\n\t<05bd0de4-5ee8-b56c-0f01-8b2115a6afd6@gmx.de>\n\t<20220402094614.y4slbqpyykm4j6te@uno.localdomain>","In-Reply-To":"<20220402094614.y4slbqpyykm4j6te@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:7SmXpu4Guo9MphOnvOU96m6F1q/Br4Wd6/TstnrFY4XPH0nNtqX\n\t+bLsoujEdrCV4oGinyt9i8pvvTojNsezFOHFYzTaBHc/Eg7tHrTJ+DM2yNHlq71p93si1HW\n\tFZizlc/wMrnH3Bi5jHp60BEAG4nodFCnlwk3nV6j4zeQdV+ZkCfME6yP4SuJvm8GCPZ0zWJ\n\t8XQP/5If/pvObEm+ofWDA==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:mFTBfYnfdvk=:Jp1Wv5OJswrNaratcjhdQN\n\tlJD0jBVtca7UdYKG6FYbrZmaJQjvqKaODTFxzDUmmLBZ10fDBMTDnkjNGhj78MtKbHLE1LkIn\n\tFqyzlVB/DLNl6zVIDt8KRSlM1Tg7NEA8cFyBUgt6pKhI1c71kCJXSTNff3BvEPko3WqbspZ3z\n\tRTa8UR8vtFNcS/WdeoCltjmHgxof68xo46qRMlCl1i6DzeXZqSRn3Kamrr5iDXVLfWYA8U0X1\n\t6sTHWSImWn6PeDb6oKKnopUdDJb2gWfj0O+RtXEx55zz+gt3DvrHzBYA10M1DnLPSjQYFSQz8\n\tutWbjC0yl2a0ebVgkobg2x97e1CmLzP0ploCMz+mUI61e8wpL2Stebcza8uj+UIswCzn44XR/\n\t8wkWHhBGDGWUtSvZb09jSOj5b3LisOhrMY4Vd0d6t7g6gE1oznZT9W5MRlDYNgDG+ZnnIihVH\n\t4EXEwyRI+eHQetWvT0fMRExZ2k6y5VjTbQrI5TxUZEx9Mj/9NjAoG3errtkxeZL0ACf9izqty\n\tq7nUfCfWkd8FUNPRjZTqJn6RNbZRI+AZLKrVrkg1MxwxS/IWi+Q0HAgKJ7xjU0KWNSl3LqYer\n\tvY18Rmr3XcyEuB1oUz3l8pL91NBGXrCBKVviEmh3pujYLSIoZXL6ACt9PcVzgeNrQlrhgmPDv\n\t28AoU+0vy5PYCQwdpa0gkANkHC4a/CcGvgTBKnTN+itpZj2oO2LbN9N1A5w2WoqZif/lVZlM5\n\twVGeAMlO4T77Sl6j14/VmZS2AfXxNbWipg09f8T7jfHjaoh7Gh35T+CiJBb4chVuW3wNQOMpO\n\tR9jbhG3p5/xbAxkLOFlQpmKJavpcd0X+cXZFEgNmYN7KD0vHQn4xpO5Cey4hpPMxLHtADOL7o\n\tM/+SDA9MMqRoKjkZKIgaYAKob2xHB06U3RIrC88QI1kh1e7lAB+DVp47QZVeTeE9topjPNKpb\n\t6DXXbvxVpGiItylz8iKdrSDYAlnJeg7lTyniS1hL3jcbsHCl33C/KxuyMQ8qADyFg56rjSKn+\n\tbl3T5i16GXxlGuVK7MD3x/eYSoq4Heb9gXSLbTw8toObkLpoI7vhIyop5C+tp8CeymX2jjOn0\n\tIxJhJLmrIp/rJQ=","Subject":"Re: [libcamera-devel] [PATCH 3/5] provide a default fixed-sized\n\tSpan constructor","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","From":"Christian Rauch via libcamera-devel\n\t","Reply-To":"Christian Rauch ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22558,"web_url":"https://patchwork.libcamera.org/comment/22558/","msgid":"<20220404103247.dq5qlexcc22pkoh5@uno.localdomain>","date":"2022-04-04T10:32:47","subject":"Re: [libcamera-devel] [PATCH 3/5] provide a default fixed-sized\n\tSpan constructor","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Christian\n + Laurent\n\nOn Sat, Apr 02, 2022 at 12:28:05PM +0100, Christian Rauch wrote:\n> Hi Jacopo,\n>\n> See my response below, specifically see the case about \"std::optional\".\n>\n> Am 02.04.22 um 10:46 schrieb Jacopo Mondi:\n> > Hi Christian,\n> >\n> > On Sat, Apr 02, 2022 at 12:05:43AM +0100, Christian Rauch wrote:\n> >> Hi Jacopo,\n> >>\n> >> Thanks for reviewing this. See my responses inline below.\n> >>\n> >>\n> >> Am 01.04.22 um 15:28 schrieb Jacopo Mondi:\n> >>> Hi Christian\n> >>>\n> >>> On Fri, Apr 01, 2022 at 01:06:14AM +0100, Christian Rauch via libcamera-devel wrote:\n> >>>> The new default constructor allows to construct a fixed-sized Span via the\n> >>>> default constructor of its stored data type.\n> >>>> This prevents the construction of empty fixed-sized Spans that cannot hold\n> >>>> any data.\n> >>>>\n> >>>> Signed-off-by: Christian Rauch \n> >>>> ---\n> >>>> include/libcamera/base/span.h | 5 +++++\n> >>>> include/libcamera/controls.h | 2 +-\n> >>>> test/span.cpp | 2 +-\n> >>>> 3 files changed, 7 insertions(+), 2 deletions(-)\n> >>>>\n> >>>> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h\n> >>>> index 88d2e3de..7a4806dc 100644\n> >>>> --- a/include/libcamera/base/span.h\n> >>>> +++ b/include/libcamera/base/span.h\n> >>>> @@ -112,6 +112,11 @@ public:\n> >>>> \t{\n> >>>> \t}\n> >>>>\n> >>>> +\tSpan()\n> >>>> +\t{\n> >>>> +\t\tSpan(std::array{});\n> >>>> +\t}\n> >>>> +\n> >>>\n> >>> This constructor creates a span of 'extent' elements all of them\n> >>> initialized to 0 then ?\n> >>\n> >> All elements will be default constructed. Integers and floats will be\n> >> initialised to 0, \"std::strings\" will be empty, and 'Size' and\n> >> 'Rectangle' will use their specified default constructors.\n> >> The default initialisation is required because the fixed-sized array\n> >> cannot be empty.\n> >>\n> >\n> > You're right, \"initialized to 0\" is not correct, \"default constructed\"\n> > is the term I should have used :)\n> >\n> >>>\n> >>> If I remove it I get\n> >>>\n> >>> ../include/libcamera/controls.h:380:34: error: no matching function for call to ‘libcamera::Span::Span()’\n> >>> 380 | return T{};\n> >>>\n> >>> Caused by\n> >>> \ttemplate\n> >>> \tT get(const Control &ctrl) const\n> >>> \t{\n> >>> \t\tconst ControlValue *val = find(ctrl.id());\n> >>> \t\tif (!val)\n> >>> \t\t\treturn T{}; <------\n> >>>\n> >>> \t\treturn val->get();\n> >>> \t}\n> >>>\n> >>> as now that extent is != 0 the expression doesn't bind anymore to the\n> >>> existing constructor:\n> >>>\n> >>> \ttemplate >>> \t\t typename = std::enable_if_t>\n> >>> \tconstexpr Span() noexcept\n> >>> \t\t: data_(nullptr)\n> >>> \t{\n> >>> \t}\n> >>>\n> >>> (I don't fully get what is the use of Dependent in the above definition :)\n> >>\n> >> I also do not understand the use of this constructor, since a\n> >> 0-fixed-size Span cannot store anything.\n> >\n> > I think it was used to identify a non-valid span, to be returned in\n> > case of errors\n> >\n> >>>\n> >>> However I feel like creating a Span of Extent elements all zeroed is a\n> >>> bit hill-defined, considering it is only used to return an error\n> >>> condition. In example, if I'm not mistaken a Span constructed with\n> >>> your proposed constructor will\n> >>>\n> >>> \tconstexpr size_type size() const noexcept { return Extent; }\n> >>> \tconstexpr size_type size_bytes() const noexcept { return size() * sizeof(element_type); }\n> >>> \tconstexpr bool empty() const noexcept { return size() == 0; }\n> >>>\n> >>> be !empty and of size == 2, which is misleading considering we really\n> >>> want to return an empty Span in case of error and the caller should\n> >>> not be mis-leaded thinking it is valid.\n> >>>\n> >>> I don't have any super smart proposal to offer, if not recording the\n> >>> 'valid' or 'empty' state in a class member and use it in size() and\n> >>> empty() instead of relying on Extent which is defined at overload\n> >>> resolution time and cannot be assigned at run time.\n> >>\n> >> I think this is a general issue with return default constructed objects\n> >> to indicate an error. The error is only defined implicitly. If\n> >> ControlList::get returns a 'Size' with width=0 and height=0, then it is\n> >> not clear if this indicates an error or if this value is really stored.\n> >\n> > Yes and no, if the custom type has a well defined interface it can be\n> > provided with an isValid()/isNull() function, like it is done for the\n> > Size class\n> >\n> > \tbool isNull() const { return !width && !height; }\n> >\n> > I understand that\n> >\n> > Size s = controlList.get(SomeSizeControl);\n> > if (s.isNull()) {\n> > /* Error out */\n> > }\n> >\n> > Requires knowledge of the custom type interface and special care as\n> > it's less intuitive than handling an int return code or a pointer, but\n> > either we forbid return-by-value function signatures completely or we\n> > have to instrument custom types with an isValid() function for the\n> > caller to test.\n>\n> But this will only ever work for custom types. Basic C types and C++\n> standard types do not have such a flag. \"isNull\" only means that the\n> area of the Size is zero, but it does not directly mean that the\n> requested \"Size\" does not exist.\n>\n\nTo me standard type are less a concern as they all have functions to test if\nthey're empty. Of course this assumes an empty vector returned by a\nfunction is an error condition, something that it's not semantically\naccurate, I agree\n\n> >\n> > The Span class validity test is based on its Extent size, and the\n> > constructor you have introduced makes an invalid Span look like it is\n> > valid.\n>\n> I really don't like this idea of implicitly encoding \"validity\" by a\n> specific value :-)\n>\n> >\n> >>\n> >> You could add special class members to indicate that an object is\n> >> \"valid\", but this is not a nice solution and only works for custom types\n> >\n> > Other custom libcamera types are fine in that regard. I was suggesting\n> > to add a 'bool valid_;' class member to Span, to not rely on Extent to\n> > test its validity.\n> >\n> >> in libcamera. The \"C\" way would probably to indicate an error by a NULL\n> >> pointer and a returned error number.\n> >>\n> >> I think the proper way would be to throw an exception. This could also\n> >> be a custom libcamera exception.\n> >>\n> >\n> > Too bad libcamera doesn't use exceptions ;)\n>\n> Why is that? Has this something to do with \"embedded C++\"? Alternatively\n\nI think among the reasons, apart from code size and efficiency on\nwhich I don't have numbers to debate on but seems sensible for some\nembedded platforms, there is the fact that using exceptions\nwould require code that use libcamera to be exception-safe, it would\nhave made more complex creating a C-API and deal with errors through\nIPC (which libcamera use heavily to communicate with IPA modules).\n\n> to exceptions, the get method could either 1) take a target value by\n> reference and return a status flag (success / failure) or 2) take a\n> status flag by reference and return a default constructed object.\n>\n> There is also \"std::optional<>\" which can express this: \"A common use\n> case for optional is the return value of a function that may fail.\"\n>\n> If you are ok with newer C++ features, than \"optional\" is maybe the most\n> \"expressive\" solution:\n>\n> std::optional ret = controlList.get(SomeSizeControl);\n> if (ret.has_value()) {\n> // OK\n> Size s = s.value();\n> }\n> else {\n> // ERROR\n> }\n>\n> What do you think about this? The \"optional\" is essentially attaching a\n> validity flag to any object returned by the \"get\" method, but it does\n> not require changing the type itself.\n>\n\nstd::optional<> has been introduced in C++17, which libcamera migrated\nto only 1.5 year ago. At the time the Control interface had been\ndesigned std::optional<> was not available, if memory does not fail me.\n\nHowever it seems a rather neat way to handle return-by-value error\nconditions and I would be in favour to move the code base to use it.\n\nWe already have a user in libcamera, introduced by RPi when adding\nsupport to color space handling\ninclude/libcamera/internal/v4l2_device.h: static std::optional toColorSpace(const T &v4l2Format);\n\nLet me rope-in Laurent by CC-ing him to know what he thinks.\n\nThanks for the discussion starter!\n\n> >\n> >>>\n> >>>> \texplicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)\n> >>>> \t\t: data_(ptr)\n> >>>> \t{\n> >>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >>>> index 665bcac1..de8a7770 100644\n> >>>> --- a/include/libcamera/controls.h\n> >>>> +++ b/include/libcamera/controls.h\n> >>>> @@ -167,7 +167,7 @@ public:\n> >>>>\n> >>>> \t\tusing V = typename T::value_type;\n> >>>> \t\tconst V *value = reinterpret_cast(data().data());\n> >>>> -\t\treturn { value, numElements_ };\n> >>>> +\t\treturn T{ value, numElements_ };\n> >>>\n> >>> this applies to overload\n> >>>\n> >>> \ttemplate::value ||\n> >>> \t\t\t\t\t\t std::is_same>::value,\n> >>> \tT get() const\n> >>> \t{\n> >>> \t\tassert(type_ == details::control_type>::value);\n> >>> \t\tassert(isArray_);\n> >>>\n> >>> \t\tusing V = typename T::value_type;\n> >>> \t\tconst V *value = reinterpret_cast(data().data());\n> >>> \t\treturn { value, numElements_ };\n> >>> \t}\n> >>>\n> >>> Isn't the returned { value, numElements_ } already used to construct an\n> >>> instance of T if assigned to an lvalue expression ?\n> >>>\n> >>> IOW why do you need to contruct an instance of T and return it\n> >>> explicitely ? Thanks to return value optimization this shouldn't have\n> >>> any performance impact but I have missed why it is needed.\n> >>>\n> >>> <2 minutes later>\n> >>>\n> >>> And now that I wrote this, if I remove it I get\n> >>>\n> >>> include/libcamera/controls.h:170:46: error: converting to\n> >>> ‘libcamera::Span’ from initializer list would use\n> >>> explicit constructor ‘constexpr libcamera::Span >>> Extent>::Span(libcamera::Span::pointer, libcamera::Span >>> Extent>::size_type) [with T = const float; long unsigned int Extent =\n> >>> 2; libcamera::Span::pointer = const float*;\n> >>> libcamera::Span::size_type = long unsigned int]’\n> >>>\n> >>> Because indeed\n> >>> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)\n> >>>\n> >>> is, well, explicit (which I think it's ok and should not be removed)\n> >>>\n> >>> I'll leave here the above text anyway for others that might have the\n> >>> same question :)\n> >>>\n> >>>> \t}\n> >>>>\n> >>>> #ifndef __DOXYGEN__\n> >>>> diff --git a/test/span.cpp b/test/span.cpp\n> >>>> index abf3a5d6..c37e2a66 100644\n> >>>> --- a/test/span.cpp\n> >>>> +++ b/test/span.cpp\n> >>>> @@ -37,7 +37,7 @@ protected:\n> >>>> \t\t * to generate undefined behaviour.\n> >>>> \t\t */\n> >>>>\n> >>>> -\t\tSpan{};\n> >>>> +\t\t/* Span{}; */\n> >>>\n> >>> You should remove it, or if the above issue with constructing an\n> >>> 'empty' Span of size > 0 is resolved, prove that it works as intended.\n> >>\n> >> I commented it out instead of removing this, because the comment above\n> >> says \"[...] Commented-out tests are expected not to compile [...]\" and\n> >> there was already a commented case for an 4-sized Span. Anyway, I don't\n> >\n> > Oh I see, sorry, missed it.\n> >\n> >> see a real application of a 0-fixed-sized Span, as it will never be able\n> >> to store anything.\n> >\n> > My understanding is that it is only used for error conditions.\n>\n> In this case, this \"error condition\" will be replaced by another\n> compiler error that will trigger for the same reasons (empty span), but\n> will be triggered by trying to construct an empty \"std::array\".\n>\n> Best,\n> Christian\n>\n> >\n> > Thanks\n> > j\n> >\n> >>\n> >> Best,\n> >> Christian\n> >>\n> >>\n> >>>\n> >>> Span s{}\n> >>> if (!s.empty())\n> >>> error!\n> >>>\n> >>> Thanks\n> >>> j\n> >>>\n> >>>> \t\t/* Span{}; */\n> >>>>\n> >>>> \t\tSpan{ &i[0], 4 };\n> >>>> --\n> >>>> 2.25.1\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 9BDF4C3256\n\tfor ;\n\tMon, 4 Apr 2022 10:32:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E35FE65642;\n\tMon, 4 Apr 2022 12:32:52 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1F95D633A5\n\tfor ;\n\tMon, 4 Apr 2022 12:32:50 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 65E4AFF803;\n\tMon, 4 Apr 2022 10:32:49 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649068372;\n\tbh=DanXGH7Mh/jRi6yEGvhPUvKKc9anVpFfTx0eLBz00Xw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=pKxfN7hRuNJw70xhgqo7cljw4T5a+BL0ndHIdsuB2zlZ9+/wMGGjt3wAXXiJjpA1w\n\tWKBX2v9w6KD4w+aUGWRGAIePcpruTEQ7l3IyVwDyea7YsB5iZu9AdwxfEjWKuVjcpX\n\t5h2GWhJKLc99lQLcx09mA66XyaNeubZp7oHzLnk0lZ9fLxg2Kw4LmGBuIHEZ/cPTUf\n\t4od9EXI/+IdawwsFB3DYZQHDTKvL5sHjNHUG3gn81wUovQJ71+rHG0QhcqRrMVRJLo\n\tSH+euqZsfiUKVW+GZ6TfRlT+nMnI+Fz/6rKofw8mn0WKdXJIbnqh8xUdblJpCm3fJJ\n\t6Z2eTVKATcHfQ==","Date":"Mon, 4 Apr 2022 12:32:47 +0200","To":"Christian Rauch ,\n\tLaurent Pinchart ","Message-ID":"<20220404103247.dq5qlexcc22pkoh5@uno.localdomain>","References":"<20220401000616.12976-1-Rauch.Christian@gmx.de>\n\t<20220401000616.12976-4-Rauch.Christian@gmx.de>\n\t<20220401142826.iqkwb3wmsyr5f7p5@uno.localdomain>\n\t<05bd0de4-5ee8-b56c-0f01-8b2115a6afd6@gmx.de>\n\t<20220402094614.y4slbqpyykm4j6te@uno.localdomain>\n\t<33eec917-3c50-ba76-5060-cd62211846f3@gmx.de>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<33eec917-3c50-ba76-5060-cd62211846f3@gmx.de>","Subject":"Re: [libcamera-devel] [PATCH 3/5] provide a default fixed-sized\n\tSpan constructor","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22559,"web_url":"https://patchwork.libcamera.org/comment/22559/","msgid":"","date":"2022-04-04T11:32:29","subject":"Re: [libcamera-devel] [PATCH 3/5] provide a default fixed-sized\n\tSpan constructor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Apr 04, 2022 at 12:32:47PM +0200, Jacopo Mondi wrote:\n> On Sat, Apr 02, 2022 at 12:28:05PM +0100, Christian Rauch wrote:\n> > Hi Jacopo,\n> >\n> > See my response below, specifically see the case about \"std::optional\".\n> >\n> > Am 02.04.22 um 10:46 schrieb Jacopo Mondi:\n> > > On Sat, Apr 02, 2022 at 12:05:43AM +0100, Christian Rauch wrote:\n> > >> Am 01.04.22 um 15:28 schrieb Jacopo Mondi:\n> > >>> On Fri, Apr 01, 2022 at 01:06:14AM +0100, Christian Rauch via libcamera-devel wrote:\n> > >>>> The new default constructor allows to construct a fixed-sized Span via the\n> > >>>> default constructor of its stored data type.\n> > >>>> This prevents the construction of empty fixed-sized Spans that cannot hold\n> > >>>> any data.\n> > >>>>\n> > >>>> Signed-off-by: Christian Rauch \n> > >>>> ---\n> > >>>> include/libcamera/base/span.h | 5 +++++\n> > >>>> include/libcamera/controls.h | 2 +-\n> > >>>> test/span.cpp | 2 +-\n> > >>>> 3 files changed, 7 insertions(+), 2 deletions(-)\n> > >>>>\n> > >>>> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h\n> > >>>> index 88d2e3de..7a4806dc 100644\n> > >>>> --- a/include/libcamera/base/span.h\n> > >>>> +++ b/include/libcamera/base/span.h\n> > >>>> @@ -112,6 +112,11 @@ public:\n> > >>>> \t{\n> > >>>> \t}\n> > >>>>\n> > >>>> +\tSpan()\n> > >>>> +\t{\n> > >>>> +\t\tSpan(std::array{});\n\nI don't think this will do what you expect. The code is equivalent to\n\n\tstd::array data{};\n\n\tSpan(data);\n\nThe first line allocates an array of extent default-initialized elements\non the stack. The second line calls the Span constructor that takes an\nstd::array argument, and the span's data_ pointer points to\narray.data(). Then, when you return from the destructor, the array\nvariable is destroyed, and data_ points to freed memory.\n\nThe Span class is modelled after std::span, introduced in C++20. When\nlibcamera will move to C++20 (I don't expect that to happen before a few\nyears), the plan is to switch to std::span. We should thus keep the Span\nimplementation as close as possible to std::span (it can't be exactly\nidentical, as std::span relies on other C++20 features, for instance\nconstructor 7 in [1] uses the C++20 range API, so we have two different\nconstructors that take a reference to a container instead).\n\n[1] https://en.cppreference.com/w/cpp/container/span/span\n\n> > >>>> +\t}\n> > >>>> +\n> > >>>\n> > >>> This constructor creates a span of 'extent' elements all of them\n> > >>> initialized to 0 then ?\n> > >>\n> > >> All elements will be default constructed. Integers and floats will be\n> > >> initialised to 0, \"std::strings\" will be empty, and 'Size' and\n> > >> 'Rectangle' will use their specified default constructors.\n> > >> The default initialisation is required because the fixed-sized array\n> > >> cannot be empty.\n> > >\n> > > You're right, \"initialized to 0\" is not correct, \"default constructed\"\n> > > is the term I should have used :)\n> > >\n> > >>> If I remove it I get\n> > >>>\n> > >>> ../include/libcamera/controls.h:380:34: error: no matching function for call to ‘libcamera::Span::Span()’\n> > >>> 380 | return T{};\n> > >>>\n> > >>> Caused by\n> > >>> \ttemplate\n> > >>> \tT get(const Control &ctrl) const\n> > >>> \t{\n> > >>> \t\tconst ControlValue *val = find(ctrl.id());\n> > >>> \t\tif (!val)\n> > >>> \t\t\treturn T{}; <------\n> > >>>\n> > >>> \t\treturn val->get();\n> > >>> \t}\n> > >>>\n> > >>> as now that extent is != 0 the expression doesn't bind anymore to the\n> > >>> existing constructor:\n> > >>>\n> > >>> \ttemplate > >>> \t\t typename = std::enable_if_t>\n> > >>> \tconstexpr Span() noexcept\n> > >>> \t\t: data_(nullptr)\n> > >>> \t{\n> > >>> \t}\n> > >>>\n> > >>> (I don't fully get what is the use of Dependent in the above definition :)\n> > >>\n> > >> I also do not understand the use of this constructor, since a\n> > >> 0-fixed-size Span cannot store anything.\n> > >\n> > > I think it was used to identify a non-valid span, to be returned in\n> > > case of errors\n> > >\n> > >>> However I feel like creating a Span of Extent elements all zeroed is a\n> > >>> bit hill-defined, considering it is only used to return an error\n> > >>> condition. In example, if I'm not mistaken a Span constructed with\n> > >>> your proposed constructor will\n> > >>>\n> > >>> \tconstexpr size_type size() const noexcept { return Extent; }\n> > >>> \tconstexpr size_type size_bytes() const noexcept { return size() * sizeof(element_type); }\n> > >>> \tconstexpr bool empty() const noexcept { return size() == 0; }\n> > >>>\n> > >>> be !empty and of size == 2, which is misleading considering we really\n> > >>> want to return an empty Span in case of error and the caller should\n> > >>> not be mis-leaded thinking it is valid.\n> > >>>\n> > >>> I don't have any super smart proposal to offer, if not recording the\n> > >>> 'valid' or 'empty' state in a class member and use it in size() and\n> > >>> empty() instead of relying on Extent which is defined at overload\n> > >>> resolution time and cannot be assigned at run time.\n> > >>\n> > >> I think this is a general issue with return default constructed objects\n> > >> to indicate an error. The error is only defined implicitly. If\n> > >> ControlList::get returns a 'Size' with width=0 and height=0, then it is\n> > >> not clear if this indicates an error or if this value is really stored.\n> > >\n> > > Yes and no, if the custom type has a well defined interface it can be\n> > > provided with an isValid()/isNull() function, like it is done for the\n> > > Size class\n> > >\n> > > \tbool isNull() const { return !width && !height; }\n> > >\n> > > I understand that\n> > >\n> > > Size s = controlList.get(SomeSizeControl);\n> > > if (s.isNull()) {\n> > > /* Error out */\n> > > }\n> > >\n> > > Requires knowledge of the custom type interface and special care as\n> > > it's less intuitive than handling an int return code or a pointer, but\n> > > either we forbid return-by-value function signatures completely or we\n> > > have to instrument custom types with an isValid() function for the\n> > > caller to test.\n> >\n> > But this will only ever work for custom types. Basic C types and C++\n> > standard types do not have such a flag. \"isNull\" only means that the\n> > area of the Size is zero, but it does not directly mean that the\n> > requested \"Size\" does not exist.\n> \n> To me standard type are less a concern as they all have functions to test if\n> they're empty. Of course this assumes an empty vector returned by a\n> function is an error condition, something that it's not semantically\n> accurate, I agree\n> \n> > > The Span class validity test is based on its Extent size, and the\n> > > constructor you have introduced makes an invalid Span look like it is\n> > > valid.\n> >\n> > I really don't like this idea of implicitly encoding \"validity\" by a\n> > specific value :-)\n> >\n> > >> You could add special class members to indicate that an object is\n> > >> \"valid\", but this is not a nice solution and only works for custom types\n> > >\n> > > Other custom libcamera types are fine in that regard. I was suggesting\n> > > to add a 'bool valid_;' class member to Span, to not rely on Extent to\n> > > test its validity.\n> > >\n> > >> in libcamera. The \"C\" way would probably to indicate an error by a NULL\n> > >> pointer and a returned error number.\n> > >>\n> > >> I think the proper way would be to throw an exception. This could also\n> > >> be a custom libcamera exception.\n> > >\n> > > Too bad libcamera doesn't use exceptions ;)\n> >\n> > Why is that? Has this something to do with \"embedded C++\"? Alternatively\n> \n> I think among the reasons, apart from code size and efficiency on\n> which I don't have numbers to debate on but seems sensible for some\n> embedded platforms, there is the fact that using exceptions\n> would require code that use libcamera to be exception-safe, it would\n> have made more complex creating a C-API and deal with errors through\n> IPC (which libcamera use heavily to communicate with IPA modules).\n> \n> > to exceptions, the get method could either 1) take a target value by\n> > reference and return a status flag (success / failure) or 2) take a\n> > status flag by reference and return a default constructed object.\n> >\n> > There is also \"std::optional<>\" which can express this: \"A common use\n> > case for optional is the return value of a function that may fail.\"\n> >\n> > If you are ok with newer C++ features, than \"optional\" is maybe the most\n> > \"expressive\" solution:\n> >\n> > std::optional ret = controlList.get(SomeSizeControl);\n> > if (ret.has_value()) {\n> > // OK\n> > Size s = s.value();\n> > }\n> > else {\n> > // ERROR\n> > }\n> >\n> > What do you think about this? The \"optional\" is essentially attaching a\n> > validity flag to any object returned by the \"get\" method, but it does\n> > not require changing the type itself.\n> \n> std::optional<> has been introduced in C++17, which libcamera migrated\n> to only 1.5 year ago. At the time the Control interface had been\n> designed std::optional<> was not available, if memory does not fail me.\n> \n> However it seems a rather neat way to handle return-by-value error\n> conditions and I would be in favour to move the code base to use it.\n\nI do like std::optional, it's a useful feature. We use it internally,\nand we can extend its usage. However, while libcamera is compiled for\nC++17, we keep the public API C++14-compatible. This was decided to\nallow applications still using C++14 to compile and link against\nlibcamera, with the main use case being Chromium (for which we have\nimplement native libcamera support in [2], not merged in mainline yet).\nIt seems that Chromium has now moved to C++17 ([3]), so we could\npossibly follow suit. We'll have to rebuild and test Chromium\nintegration on the latest version first.\n\nThis being said, I would prefer keeping the public API C++14-compatible\nfor a bit longer if possible, as not all applications have moved to\nC++17. If C++17 feature would be so useful in the public API that\ndropping support for C++14 users would be an acceptable drawback, I'm\nnot against that\n\n[2] https://github.com/libcamera-org/chromium\n[3] https://bugs.chromium.org/p/chromium/issues/detail?id=752720\n\n> We already have a user in libcamera, introduced by RPi when adding\n> support to color space handling\n> include/libcamera/internal/v4l2_device.h: static std::optional toColorSpace(const T &v4l2Format);\n> \n> Let me rope-in Laurent by CC-ing him to know what he thinks.\n> \n> Thanks for the discussion starter!\n>\n> > >>>> \texplicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)\n> > >>>> \t\t: data_(ptr)\n> > >>>> \t{\n> > >>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > >>>> index 665bcac1..de8a7770 100644\n> > >>>> --- a/include/libcamera/controls.h\n> > >>>> +++ b/include/libcamera/controls.h\n> > >>>> @@ -167,7 +167,7 @@ public:\n> > >>>>\n> > >>>> \t\tusing V = typename T::value_type;\n> > >>>> \t\tconst V *value = reinterpret_cast(data().data());\n> > >>>> -\t\treturn { value, numElements_ };\n> > >>>> +\t\treturn T{ value, numElements_ };\n> > >>>\n> > >>> this applies to overload\n> > >>>\n> > >>> \ttemplate::value ||\n> > >>> \t\t\t\t\t\t std::is_same>::value,\n> > >>> \tT get() const\n> > >>> \t{\n> > >>> \t\tassert(type_ == details::control_type>::value);\n> > >>> \t\tassert(isArray_);\n> > >>>\n> > >>> \t\tusing V = typename T::value_type;\n> > >>> \t\tconst V *value = reinterpret_cast(data().data());\n> > >>> \t\treturn { value, numElements_ };\n> > >>> \t}\n> > >>>\n> > >>> Isn't the returned { value, numElements_ } already used to construct an\n> > >>> instance of T if assigned to an lvalue expression ?\n> > >>>\n> > >>> IOW why do you need to contruct an instance of T and return it\n> > >>> explicitely ? Thanks to return value optimization this shouldn't have\n> > >>> any performance impact but I have missed why it is needed.\n> > >>>\n> > >>> <2 minutes later>\n> > >>>\n> > >>> And now that I wrote this, if I remove it I get\n> > >>>\n> > >>> include/libcamera/controls.h:170:46: error: converting to\n> > >>> ‘libcamera::Span’ from initializer list would use\n> > >>> explicit constructor ‘constexpr libcamera::Span > >>> Extent>::Span(libcamera::Span::pointer, libcamera::Span > >>> Extent>::size_type) [with T = const float; long unsigned int Extent =\n> > >>> 2; libcamera::Span::pointer = const float*;\n> > >>> libcamera::Span::size_type = long unsigned int]’\n> > >>>\n> > >>> Because indeed\n> > >>> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)\n> > >>>\n> > >>> is, well, explicit (which I think it's ok and should not be removed)\n> > >>>\n> > >>> I'll leave here the above text anyway for others that might have the\n> > >>> same question :)\n> > >>>\n> > >>>> \t}\n> > >>>>\n> > >>>> #ifndef __DOXYGEN__\n> > >>>> diff --git a/test/span.cpp b/test/span.cpp\n> > >>>> index abf3a5d6..c37e2a66 100644\n> > >>>> --- a/test/span.cpp\n> > >>>> +++ b/test/span.cpp\n> > >>>> @@ -37,7 +37,7 @@ protected:\n> > >>>> \t\t * to generate undefined behaviour.\n> > >>>> \t\t */\n> > >>>>\n> > >>>> -\t\tSpan{};\n> > >>>> +\t\t/* Span{}; */\n> > >>>\n> > >>> You should remove it, or if the above issue with constructing an\n> > >>> 'empty' Span of size > 0 is resolved, prove that it works as intended.\n> > >>\n> > >> I commented it out instead of removing this, because the comment above\n> > >> says \"[...] Commented-out tests are expected not to compile [...]\" and\n> > >> there was already a commented case for an 4-sized Span. Anyway, I don't\n> > >\n> > > Oh I see, sorry, missed it.\n> > >\n> > >> see a real application of a 0-fixed-sized Span, as it will never be able\n> > >> to store anything.\n> > >\n> > > My understanding is that it is only used for error conditions.\n> >\n> > In this case, this \"error condition\" will be replaced by another\n> > compiler error that will trigger for the same reasons (empty span), but\n> > will be triggered by trying to construct an empty \"std::array\".\n> >\n> > >>> Span s{}\n> > >>> if (!s.empty())\n> > >>> error!\n> > >>>\n> > >>>> \t\t/* Span{}; */\n> > >>>>\n> > >>>> \t\tSpan{ &i[0], 4 };","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 247B8C0F1B\n\tfor ;\n\tMon, 4 Apr 2022 11:32:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8412465644;\n\tMon, 4 Apr 2022 13:32:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8896B633A5\n\tfor ;\n\tMon, 4 Apr 2022 13:32:32 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E8FB5486;\n\tMon, 4 Apr 2022 13:32:31 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649071953;\n\tbh=BeCHAyGPgAuHgL0M2baV6wcErOmTYvsAEU+5sfB9dnw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=txpYvl1ctqojLzCZQLupLwnSoRjgeWADs7TjDBEGC4rO+CiDj9FKVEKO5GjKQ81UJ\n\tWfyU/8gX4A+xpuJWVD4on0Yo9j78eMcDv5UIqymVmPbJfmur4x7JUmEg5ZyCyaC1A6\n\tEVWjNSA4S2HB7Qnazzy6ukigkZVOT3GsAO6YuriXkcDvmjNI1gvQPaWk5IgB2c481a\n\totE4eYTE0R8QFSDch5D6Br4SAtrPedLhPe0oiGnGHKR/z0yJT+xzH2ql/PP44PYlzW\n\tzTWQbUas30k4vjRfX5cTdr202mYF8xtNI7NfVGS1U08pbuSk4wqz7Q2OKMHEzvkIKK\n\tmeY0hDVCfuZOg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649071952;\n\tbh=BeCHAyGPgAuHgL0M2baV6wcErOmTYvsAEU+5sfB9dnw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MPROd4ygpm3PytCgTiA9iapBXgI5OItBaVkhVhinfPF0l3aI7pGRvV/LX+DaVW7vO\n\tKaEGxaQVpvlmht69rex8i1eWQyG1f/dP1axwL9mMbWd8ZRq4PddRqjLGnOy/kiuRhc\n\taszcl8b7ethdufSHG5x6skeBRi/FaD9jL5Mtu+hk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"MPROd4yg\"; dkim-atps=neutral","Date":"Mon, 4 Apr 2022 14:32:29 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220401000616.12976-1-Rauch.Christian@gmx.de>\n\t<20220401000616.12976-4-Rauch.Christian@gmx.de>\n\t<20220401142826.iqkwb3wmsyr5f7p5@uno.localdomain>\n\t<05bd0de4-5ee8-b56c-0f01-8b2115a6afd6@gmx.de>\n\t<20220402094614.y4slbqpyykm4j6te@uno.localdomain>\n\t<33eec917-3c50-ba76-5060-cd62211846f3@gmx.de>\n\t<20220404103247.dq5qlexcc22pkoh5@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20220404103247.dq5qlexcc22pkoh5@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 3/5] provide a default fixed-sized\n\tSpan constructor","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22562,"web_url":"https://patchwork.libcamera.org/comment/22562/","msgid":"","date":"2022-04-05T00:46:31","subject":"Re: [libcamera-devel] [PATCH 3/5] provide a default fixed-sized\n\tSpan constructor","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Dear Laurent,\n\nThanks for pointing out that the \"std::array\" in my patch is only\ntemporarily constructed. I overlooked that the Span only stores the\npointer to the data and the size, and not the actual data.\n\nThis, and other minor issues, have been fixed in version 2 of my patch set.\n\nBest,\nChristian\n\n\nAm 04.04.22 um 12:32 schrieb Laurent Pinchart:\n> Hello,\n>\n> On Mon, Apr 04, 2022 at 12:32:47PM +0200, Jacopo Mondi wrote:\n>> On Sat, Apr 02, 2022 at 12:28:05PM +0100, Christian Rauch wrote:\n>>> Hi Jacopo,\n>>>\n>>> See my response below, specifically see the case about \"std::optional\".\n>>>\n>>> Am 02.04.22 um 10:46 schrieb Jacopo Mondi:\n>>>> On Sat, Apr 02, 2022 at 12:05:43AM +0100, Christian Rauch wrote:\n>>>>> Am 01.04.22 um 15:28 schrieb Jacopo Mondi:\n>>>>>> On Fri, Apr 01, 2022 at 01:06:14AM +0100, Christian Rauch via libcamera-devel wrote:\n>>>>>>> The new default constructor allows to construct a fixed-sized Span via the\n>>>>>>> default constructor of its stored data type.\n>>>>>>> This prevents the construction of empty fixed-sized Spans that cannot hold\n>>>>>>> any data.\n>>>>>>>\n>>>>>>> Signed-off-by: Christian Rauch \n>>>>>>> ---\n>>>>>>> include/libcamera/base/span.h | 5 +++++\n>>>>>>> include/libcamera/controls.h | 2 +-\n>>>>>>> test/span.cpp | 2 +-\n>>>>>>> 3 files changed, 7 insertions(+), 2 deletions(-)\n>>>>>>>\n>>>>>>> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h\n>>>>>>> index 88d2e3de..7a4806dc 100644\n>>>>>>> --- a/include/libcamera/base/span.h\n>>>>>>> +++ b/include/libcamera/base/span.h\n>>>>>>> @@ -112,6 +112,11 @@ public:\n>>>>>>> \t{\n>>>>>>> \t}\n>>>>>>>\n>>>>>>> +\tSpan()\n>>>>>>> +\t{\n>>>>>>> +\t\tSpan(std::array{});\n>\n> I don't think this will do what you expect. The code is equivalent to\n>\n> \tstd::array data{};\n>\n> \tSpan(data);\n>\n> The first line allocates an array of extent default-initialized elements\n> on the stack. The second line calls the Span constructor that takes an\n> std::array argument, and the span's data_ pointer points to\n> array.data(). Then, when you return from the destructor, the array\n> variable is destroyed, and data_ points to freed memory.\n>\n> The Span class is modelled after std::span, introduced in C++20. When\n> libcamera will move to C++20 (I don't expect that to happen before a few\n> years), the plan is to switch to std::span. We should thus keep the Span\n> implementation as close as possible to std::span (it can't be exactly\n> identical, as std::span relies on other C++20 features, for instance\n> constructor 7 in [1] uses the C++20 range API, so we have two different\n> constructors that take a reference to a container instead).\n>\n> [1] https://en.cppreference.com/w/cpp/container/span/span\n>\n>>>>>>> +\t}\n>>>>>>> +\n>>>>>>\n>>>>>> This constructor creates a span of 'extent' elements all of them\n>>>>>> initialized to 0 then ?\n>>>>>\n>>>>> All elements will be default constructed. Integers and floats will be\n>>>>> initialised to 0, \"std::strings\" will be empty, and 'Size' and\n>>>>> 'Rectangle' will use their specified default constructors.\n>>>>> The default initialisation is required because the fixed-sized array\n>>>>> cannot be empty.\n>>>>\n>>>> You're right, \"initialized to 0\" is not correct, \"default constructed\"\n>>>> is the term I should have used :)\n>>>>\n>>>>>> If I remove it I get\n>>>>>>\n>>>>>> ../include/libcamera/controls.h:380:34: error: no matching function for call to ‘libcamera::Span::Span()’\n>>>>>> 380 | return T{};\n>>>>>>\n>>>>>> Caused by\n>>>>>> \ttemplate\n>>>>>> \tT get(const Control &ctrl) const\n>>>>>> \t{\n>>>>>> \t\tconst ControlValue *val = find(ctrl.id());\n>>>>>> \t\tif (!val)\n>>>>>> \t\t\treturn T{}; <------\n>>>>>>\n>>>>>> \t\treturn val->get();\n>>>>>> \t}\n>>>>>>\n>>>>>> as now that extent is != 0 the expression doesn't bind anymore to the\n>>>>>> existing constructor:\n>>>>>>\n>>>>>> \ttemplate>>>>> \t\t typename = std::enable_if_t>\n>>>>>> \tconstexpr Span() noexcept\n>>>>>> \t\t: data_(nullptr)\n>>>>>> \t{\n>>>>>> \t}\n>>>>>>\n>>>>>> (I don't fully get what is the use of Dependent in the above definition :)\n>>>>>\n>>>>> I also do not understand the use of this constructor, since a\n>>>>> 0-fixed-size Span cannot store anything.\n>>>>\n>>>> I think it was used to identify a non-valid span, to be returned in\n>>>> case of errors\n>>>>\n>>>>>> However I feel like creating a Span of Extent elements all zeroed is a\n>>>>>> bit hill-defined, considering it is only used to return an error\n>>>>>> condition. In example, if I'm not mistaken a Span constructed with\n>>>>>> your proposed constructor will\n>>>>>>\n>>>>>> \tconstexpr size_type size() const noexcept { return Extent; }\n>>>>>> \tconstexpr size_type size_bytes() const noexcept { return size() * sizeof(element_type); }\n>>>>>> \tconstexpr bool empty() const noexcept { return size() == 0; }\n>>>>>>\n>>>>>> be !empty and of size == 2, which is misleading considering we really\n>>>>>> want to return an empty Span in case of error and the caller should\n>>>>>> not be mis-leaded thinking it is valid.\n>>>>>>\n>>>>>> I don't have any super smart proposal to offer, if not recording the\n>>>>>> 'valid' or 'empty' state in a class member and use it in size() and\n>>>>>> empty() instead of relying on Extent which is defined at overload\n>>>>>> resolution time and cannot be assigned at run time.\n>>>>>\n>>>>> I think this is a general issue with return default constructed objects\n>>>>> to indicate an error. The error is only defined implicitly. If\n>>>>> ControlList::get returns a 'Size' with width=0 and height=0, then it is\n>>>>> not clear if this indicates an error or if this value is really stored.\n>>>>\n>>>> Yes and no, if the custom type has a well defined interface it can be\n>>>> provided with an isValid()/isNull() function, like it is done for the\n>>>> Size class\n>>>>\n>>>> \tbool isNull() const { return !width && !height; }\n>>>>\n>>>> I understand that\n>>>>\n>>>> Size s = controlList.get(SomeSizeControl);\n>>>> if (s.isNull()) {\n>>>> /* Error out */\n>>>> }\n>>>>\n>>>> Requires knowledge of the custom type interface and special care as\n>>>> it's less intuitive than handling an int return code or a pointer, but\n>>>> either we forbid return-by-value function signatures completely or we\n>>>> have to instrument custom types with an isValid() function for the\n>>>> caller to test.\n>>>\n>>> But this will only ever work for custom types. Basic C types and C++\n>>> standard types do not have such a flag. \"isNull\" only means that the\n>>> area of the Size is zero, but it does not directly mean that the\n>>> requested \"Size\" does not exist.\n>>\n>> To me standard type are less a concern as they all have functions to test if\n>> they're empty. Of course this assumes an empty vector returned by a\n>> function is an error condition, something that it's not semantically\n>> accurate, I agree\n>>\n>>>> The Span class validity test is based on its Extent size, and the\n>>>> constructor you have introduced makes an invalid Span look like it is\n>>>> valid.\n>>>\n>>> I really don't like this idea of implicitly encoding \"validity\" by a\n>>> specific value :-)\n>>>\n>>>>> You could add special class members to indicate that an object is\n>>>>> \"valid\", but this is not a nice solution and only works for custom types\n>>>>\n>>>> Other custom libcamera types are fine in that regard. I was suggesting\n>>>> to add a 'bool valid_;' class member to Span, to not rely on Extent to\n>>>> test its validity.\n>>>>\n>>>>> in libcamera. The \"C\" way would probably to indicate an error by a NULL\n>>>>> pointer and a returned error number.\n>>>>>\n>>>>> I think the proper way would be to throw an exception. This could also\n>>>>> be a custom libcamera exception.\n>>>>\n>>>> Too bad libcamera doesn't use exceptions ;)\n>>>\n>>> Why is that? Has this something to do with \"embedded C++\"? Alternatively\n>>\n>> I think among the reasons, apart from code size and efficiency on\n>> which I don't have numbers to debate on but seems sensible for some\n>> embedded platforms, there is the fact that using exceptions\n>> would require code that use libcamera to be exception-safe, it would\n>> have made more complex creating a C-API and deal with errors through\n>> IPC (which libcamera use heavily to communicate with IPA modules).\n>>\n>>> to exceptions, the get method could either 1) take a target value by\n>>> reference and return a status flag (success / failure) or 2) take a\n>>> status flag by reference and return a default constructed object.\n>>>\n>>> There is also \"std::optional<>\" which can express this: \"A common use\n>>> case for optional is the return value of a function that may fail.\"\n>>>\n>>> If you are ok with newer C++ features, than \"optional\" is maybe the most\n>>> \"expressive\" solution:\n>>>\n>>> std::optional ret = controlList.get(SomeSizeControl);\n>>> if (ret.has_value()) {\n>>> // OK\n>>> Size s = s.value();\n>>> }\n>>> else {\n>>> // ERROR\n>>> }\n>>>\n>>> What do you think about this? The \"optional\" is essentially attaching a\n>>> validity flag to any object returned by the \"get\" method, but it does\n>>> not require changing the type itself.\n>>\n>> std::optional<> has been introduced in C++17, which libcamera migrated\n>> to only 1.5 year ago. At the time the Control interface had been\n>> designed std::optional<> was not available, if memory does not fail me.\n>>\n>> However it seems a rather neat way to handle return-by-value error\n>> conditions and I would be in favour to move the code base to use it.\n>\n> I do like std::optional, it's a useful feature. We use it internally,\n> and we can extend its usage. However, while libcamera is compiled for\n> C++17, we keep the public API C++14-compatible. This was decided to\n> allow applications still using C++14 to compile and link against\n> libcamera, with the main use case being Chromium (for which we have\n> implement native libcamera support in [2], not merged in mainline yet).\n> It seems that Chromium has now moved to C++17 ([3]), so we could\n> possibly follow suit. We'll have to rebuild and test Chromium\n> integration on the latest version first.\n>\n> This being said, I would prefer keeping the public API C++14-compatible\n> for a bit longer if possible, as not all applications have moved to\n> C++17. If C++17 feature would be so useful in the public API that\n> dropping support for C++14 users would be an acceptable drawback, I'm\n> not against that\n>\n> [2] https://github.com/libcamera-org/chromium\n> [3] https://bugs.chromium.org/p/chromium/issues/detail?id=752720\n>\n>> We already have a user in libcamera, introduced by RPi when adding\n>> support to color space handling\n>> include/libcamera/internal/v4l2_device.h: static std::optional toColorSpace(const T &v4l2Format);\n>>\n>> Let me rope-in Laurent by CC-ing him to know what he thinks.\n>>\n>> Thanks for the discussion starter!\n>>\n>>>>>>> \texplicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)\n>>>>>>> \t\t: data_(ptr)\n>>>>>>> \t{\n>>>>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>>>>>> index 665bcac1..de8a7770 100644\n>>>>>>> --- a/include/libcamera/controls.h\n>>>>>>> +++ b/include/libcamera/controls.h\n>>>>>>> @@ -167,7 +167,7 @@ public:\n>>>>>>>\n>>>>>>> \t\tusing V = typename T::value_type;\n>>>>>>> \t\tconst V *value = reinterpret_cast(data().data());\n>>>>>>> -\t\treturn { value, numElements_ };\n>>>>>>> +\t\treturn T{ value, numElements_ };\n>>>>>>\n>>>>>> this applies to overload\n>>>>>>\n>>>>>> \ttemplate::value ||\n>>>>>> \t\t\t\t\t\t std::is_same>::value,\n>>>>>> \tT get() const\n>>>>>> \t{\n>>>>>> \t\tassert(type_ == details::control_type>::value);\n>>>>>> \t\tassert(isArray_);\n>>>>>>\n>>>>>> \t\tusing V = typename T::value_type;\n>>>>>> \t\tconst V *value = reinterpret_cast(data().data());\n>>>>>> \t\treturn { value, numElements_ };\n>>>>>> \t}\n>>>>>>\n>>>>>> Isn't the returned { value, numElements_ } already used to construct an\n>>>>>> instance of T if assigned to an lvalue expression ?\n>>>>>>\n>>>>>> IOW why do you need to contruct an instance of T and return it\n>>>>>> explicitely ? Thanks to return value optimization this shouldn't have\n>>>>>> any performance impact but I have missed why it is needed.\n>>>>>>\n>>>>>> <2 minutes later>\n>>>>>>\n>>>>>> And now that I wrote this, if I remove it I get\n>>>>>>\n>>>>>> include/libcamera/controls.h:170:46: error: converting to\n>>>>>> ‘libcamera::Span’ from initializer list would use\n>>>>>> explicit constructor ‘constexpr libcamera::Span>>>>> Extent>::Span(libcamera::Span::pointer, libcamera::Span>>>>> Extent>::size_type) [with T = const float; long unsigned int Extent =\n>>>>>> 2; libcamera::Span::pointer = const float*;\n>>>>>> libcamera::Span::size_type = long unsigned int]’\n>>>>>>\n>>>>>> Because indeed\n>>>>>> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)\n>>>>>>\n>>>>>> is, well, explicit (which I think it's ok and should not be removed)\n>>>>>>\n>>>>>> I'll leave here the above text anyway for others that might have the\n>>>>>> same question :)\n>>>>>>\n>>>>>>> \t}\n>>>>>>>\n>>>>>>> #ifndef __DOXYGEN__\n>>>>>>> diff --git a/test/span.cpp b/test/span.cpp\n>>>>>>> index abf3a5d6..c37e2a66 100644\n>>>>>>> --- a/test/span.cpp\n>>>>>>> +++ b/test/span.cpp\n>>>>>>> @@ -37,7 +37,7 @@ protected:\n>>>>>>> \t\t * to generate undefined behaviour.\n>>>>>>> \t\t */\n>>>>>>>\n>>>>>>> -\t\tSpan{};\n>>>>>>> +\t\t/* Span{}; */\n>>>>>>\n>>>>>> You should remove it, or if the above issue with constructing an\n>>>>>> 'empty' Span of size > 0 is resolved, prove that it works as intended.\n>>>>>\n>>>>> I commented it out instead of removing this, because the comment above\n>>>>> says \"[...] Commented-out tests are expected not to compile [...]\" and\n>>>>> there was already a commented case for an 4-sized Span. Anyway, I don't\n>>>>\n>>>> Oh I see, sorry, missed it.\n>>>>\n>>>>> see a real application of a 0-fixed-sized Span, as it will never be able\n>>>>> to store anything.\n>>>>\n>>>> My understanding is that it is only used for error conditions.\n>>>\n>>> In this case, this \"error condition\" will be replaced by another\n>>> compiler error that will trigger for the same reasons (empty span), but\n>>> will be triggered by trying to construct an empty \"std::array\".\n>>>\n>>>>>> Span s{}\n>>>>>> if (!s.empty())\n>>>>>> error!\n>>>>>>\n>>>>>>> \t\t/* Span{}; */\n>>>>>>>\n>>>>>>> \t\tSpan{ &i[0], 4 };\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 C573CC3256\n\tfor ;\n\tTue, 5 Apr 2022 00:46:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3931765642;\n\tTue, 5 Apr 2022 02:46:37 +0200 (CEST)","from mout.gmx.net (mout.gmx.net [212.227.17.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3466060397\n\tfor ;\n\tTue, 5 Apr 2022 02:46:35 +0200 (CEST)","from [192.168.1.209] ([92.10.251.63]) by mail.gmx.net (mrgmx105\n\t[212.227.17.168]) with ESMTPSA (Nemesis) id 1N9dsb-1o76yt1W69-015c5t;\n\tTue, 05 Apr 2022 02:46:34 +0200"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649119597;\n\tbh=0NwGK2Y/qcofQKojOUB+bVKzHjxwU7vRXDCCV+IoNZE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=tNetgofYZl6qOrEsB5LMfAzrunEW5w+//1EvVN2mMYcK/fhnaNVT6TvdMM3auxWae\n\tk5jaQasgWKnh71NtpGDfSqMmueqTWncyaKEiaE83yB9msVheArrsNfB+BXMGk1614/\n\teDaYVRkMJXQ24oD9XTn/tQMSpFd8SaUjYD9Nje/HMFWzXHrqNiUFukATINEwloY+wA\n\tz03YS9VirHScPpxMGxdeXiqgckgk9jBriCmTGRwZI5uJuO3XJjiTqHqtIuk59jGmWO\n\t+4feEV/JRzTBDI585hsgN2HshIYjNP1p6BsGhIXZFNx9FddPOj1SNuDwxInkGl+smQ\n\tJ3bM30vj2P+IA==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1649119594;\n\tbh=0NwGK2Y/qcofQKojOUB+bVKzHjxwU7vRXDCCV+IoNZE=;\n\th=X-UI-Sender-Class:Date:Subject:To:Cc:References:From:In-Reply-To;\n\tb=fEmWaDXadMOnLrrXAZ4TsIXfRty9loPu+RooSDDgyS8EYjPo3iGH+itmeRdxF/EMx\n\twW/y1iOoJ4JkmpmVezu4+dKYQdtzRlzxRMzxVzciTFJmvM2JDEphpOuIWgNJn1rUQn\n\t1ZFCCaPXc9QGIo5dvP3RiPU5hiC+u+stE6UH8MFQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"fEmWaDXa\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"","Date":"Tue, 5 Apr 2022 01:46:31 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.7.0","Content-Language":"en-CA","To":"Laurent Pinchart ,\n\tJacopo Mondi ","References":"<20220401000616.12976-1-Rauch.Christian@gmx.de>\n\t<20220401000616.12976-4-Rauch.Christian@gmx.de>\n\t<20220401142826.iqkwb3wmsyr5f7p5@uno.localdomain>\n\t<05bd0de4-5ee8-b56c-0f01-8b2115a6afd6@gmx.de>\n\t<20220402094614.y4slbqpyykm4j6te@uno.localdomain>\n\t<33eec917-3c50-ba76-5060-cd62211846f3@gmx.de>\n\t<20220404103247.dq5qlexcc22pkoh5@uno.localdomain>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:Ed9sAkYGNtRSkA8TNXo46kA91CWba20FaTAqcCt0vQkun+5AcLc\n\tEfrHtbLVcgjTsBUiTIbD55V21MXRSPB7zpwdor04FQML6bO69klK4Ie6FDQhH8YC4/Ojiua\n\tttyHVgj7uNNB1H/6K2jaNv/tSCXsoCfDx0aC3HqBLYmLosx3pjSx7r5HrnprXxYNjhSQwOR\n\tbaR2eAo7nUq2jiV6LfbQA==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:V8Uq1dFqdPk=:5XkeJNhCOcAqbaFzV4hd0d\n\trseg6vOCQD0CUdHODOZSq+lze35XOpMelDCYEBkrx+MOpxXMJ4IoAZ4A9kyOq836yJqd3SIXs\n\txDl5StmCoR6Bx3wzc8hW8lK6WVwt8Ycsq4SZ8qyzZB4T3Fyk9h6zYURVHusNrexnT4KqUEdTL\n\tng6rMvDZBFTCrkpkrrNRZo/oMzpG7As018bOEiOvxMqXcdPKa/P7cmv9fzXV8n4KsO1WqxaCO\n\tHPPDvJ/+Da+zoNHZ6A8v7Ujmg6OklV+SU+KeKE8P1XxgPgpiAt2peaxCyCSCAU7MlXcQhwQdQ\n\tGgfC8zJ7hVoaupHCMI28p5HACvGRh8VPT4tpCNTAGb3Uqw7e7jMjpxAvsoYXYYBBHlCr7eKHj\n\tPp3z61TKCtG6y2zp2qv69zoFla3Sq9y3mQoK0077zscpDxl3bOURVORylPBWG8fKM7AJiY7PX\n\tGUXOxj967vQ9pmaH3jDjTXIjpS5E1XJ6wBz4yCBsQegA3KHkhiXOTTC1xVtZ6V/m0ZlXW/8hi\n\tbkQZoZqDKvu9m2CJi3cblX8s7GAqdPD27YbKXigvRhmbsH4xQuZoRTMgVPD3cma+guha5gLW5\n\tB1VOZoQhNYUpYG5IyMYo92XugOLS5bxcfivR0Fz2DA9bko+73IvOtJF9avRjS0gkZBDKMcoq5\n\t1oNhdjoxh8CK7W5pMqzouRJqJedn5cR0vmahYrxCYUJ4Hup2GBgORqFZaCxTFQvsJpFZ49Rvu\n\tz40LJFc10juO4cX3qGXvyAH/hVDw9X73zCYg2anOhk8N53nLMoAPeJxuoxE3w7n94BQj5mU/3\n\tn9J58XuFXj4zdBp5oRsOK7nhbjkdserkoL7x+a+qXF9FTbgqxZEIx/OTgAZ9IFjDM/HR+v1+h\n\tsBO4VK4Qf0jYMwU/MTboVfTrXoamAf5KBM4v3VeUSWGbY4NuObVubG9KGE4P2ZmwiQpkdvVcJ\n\t94qA3j1Zjbnr72LgtljfDylzjzTR1rawAfg+LHYsy2i6di174oqTLlgG94iMms/l+vIq4qAO0\n\tb5Y1BVqxqRjmHPfXo9sijjT4LJIvcJI4FKEo61y6EjzZ//wGXfogD2/nxHVNY2aFe+qeHVtbJ\n\tUu9wjqjezHliBE=","Subject":"Re: [libcamera-devel] [PATCH 3/5] provide a default fixed-sized\n\tSpan constructor","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","From":"Christian Rauch via libcamera-devel\n\t","Reply-To":"Christian Rauch ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]