[{"id":33871,"web_url":"https://patchwork.libcamera.org/comment/33871/","msgid":"<20250401214715.GI3494@pendragon.ideasonboard.com>","date":"2025-04-01T21:47:15","subject":"Re: [PATCH v1 4/4] libcamera: controls: Simplify\n\tControlValue::{ControlValue, get, set}","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Tue, Apr 01, 2025 at 03:19:39PM +0200, Barnabás Pőcze wrote:\n> Whether or not the control has an array type can be determined from the\n> static information in `detail::control_type`, so use that and\n> `if constexpr` to deduplicate the constructor, get, and set functions.\n\nI'd rather not do this, as it would prevent us from supporting arrays of\nstrings. Is there anything you're working on that depends on this\nseries, or is it standalone cleanups ?\n\n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/controls.h | 75 ++++++++++--------------------------\n> 1 file changed, 20 insertions(+), 55 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 85c724ec1..22b5e3d96 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -136,28 +136,14 @@ public:\n> \tControlValue();\n> \n> #ifndef __DOXYGEN__\n> -\ttemplate::value &&\n> -\t\t\t\t\t details::control_type::value &&\n> -\t\t\t\t\t !std::is_same>::value,\n> -\t\t\t\t\t std::nullptr_t> = nullptr>\n> -\tControlValue(const T &value)\n> -\t\t: type_(ControlTypeNone), numElements_(0)\n> -\t{\n> -\t\tset(details::control_type>::value, false,\n> -\t\t &value, 1, sizeof(T));\n> -\t}\n> -\n> -\ttemplate::value ||\n> -\t\t\t\t\t std::is_same>::value,\n> -\t\t\t\t\t std::nullptr_t> = nullptr>\n> +\ttemplate::value)>>\n> #else\n> \ttemplate\n> #endif\n> \tControlValue(const T &value)\n> -\t\t: type_(ControlTypeNone), numElements_(0)\n> +\t\t: type_(ControlTypeNone), isArray_(false), numElements_(0)\n> \t{\n> -\t\tset(details::control_type>::value, true,\n> -\t\t value.data(), value.size(), sizeof(typename T::value_type));\n> +\t\tset(value);\n> \t}\n> \n> \t~ControlValue();\n> @@ -180,54 +166,33 @@ public:\n> \t\treturn !(*this == other);\n> \t}\n> \n> -#ifndef __DOXYGEN__\n> -\ttemplate::value &&\n> -\t\t\t\t\t !std::is_same>::value,\n> -\t\t\t\t\t std::nullptr_t> = nullptr>\n> -\tT get() const\n> -\t{\n> -\t\tassert(type_ == details::control_type>::value);\n> -\t\tassert(!isArray_);\n> -\n> -\t\treturn *reinterpret_cast(data().data());\n> -\t}\n> -\n> -\ttemplate::value ||\n> -\t\t\t\t\t std::is_same>::value,\n> -\t\t\t\t\t std::nullptr_t> = nullptr>\n> -#else\n> \ttemplate\n> -#endif\n> \tT get() const\n> \t{\n> -\t\tassert(type_ == details::control_type>::value);\n> -\t\tassert(isArray_);\n> +\t\tusing TypeInfo = details::control_type>;\n> \n> -\t\tusing V = typename T::value_type;\n> -\t\tconst V *value = reinterpret_cast(data().data());\n> -\t\treturn T{ value, numElements_ };\n> -\t}\n> +\t\tassert(type_ == TypeInfo::value);\n> +\t\tassert(isArray_ == (TypeInfo::size > 0));\n> \n> -#ifndef __DOXYGEN__\n> -\ttemplate::value &&\n> -\t\t\t\t\t !std::is_same>::value,\n> -\t\t\t\t\t std::nullptr_t> = nullptr>\n> -\tvoid set(const T &value)\n> -\t{\n> -\t\tset(details::control_type>::value, false,\n> -\t\t reinterpret_cast(&value), 1, sizeof(T));\n> +\t\tif constexpr (TypeInfo::size > 0)\n> +\t\t\treturn T(reinterpret_cast(data().data()), numElements_);\n> +\t\telse\n> +\t\t\treturn *reinterpret_cast(data().data());\n> \t}\n> \n> -\ttemplate::value ||\n> -\t\t\t\t\t std::is_same>::value,\n> -\t\t\t\t\t std::nullptr_t> = nullptr>\n> -#else\n> \ttemplate\n> -#endif\n> \tvoid set(const T &value)\n> \t{\n> -\t\tset(details::control_type>::value, true,\n> -\t\t value.data(), value.size(), sizeof(typename T::value_type));\n> +\t\tusing TypeInfo = details::control_type>;\n> +\t\tconstexpr auto type = TypeInfo::value;\n> +\n> +\t\tif constexpr (TypeInfo::size > 0) {\n> +\t\t\tstatic_assert(std::is_trivially_copyable_v);\n> +\t\t\tset(type, true, value.data(), value.size(), sizeof(typename T::value_type));\n> +\t\t} else {\n> +\t\t\tstatic_assert(std::is_trivially_copyable_v);\n> +\t\t\tset(type, false, reinterpret_cast(&value), 1, sizeof(T));\n> +\t\t}\n> \t}\n> \n> \tvoid reserve(ControlType type, bool isArray = false,","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 CC714C3213\n\tfor ;\n\tTue, 1 Apr 2025 21:47:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 38DC068962;\n\tTue, 1 Apr 2025 23:47:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BAFA168947\n\tfor ;\n\tTue, 1 Apr 2025 23:47:42 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C9552741;\n\tTue, 1 Apr 2025 23:45:49 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"v/XMeVTd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743543950;\n\tbh=HVt3AJfzK4n8szMB0pNUsGsXdfCg3p1YLlbSZY7bu28=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v/XMeVTdS00cd09Nhj6dXnHHVe8MHi4JJ7ouYfxVVftvSMsiaiorpaczLoDaoWiuc\n\tW+epR45UUTj21qGCajcUwaEpkFw9WavUOtE9hKB1TAKPd61lN3PySmipHcDhvO6wRT\n\tVeoxIAggqd/2b3idzQylfQfmzXdEzz2NV1KsrvAs=","Date":"Wed, 2 Apr 2025 00:47:15 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 4/4] libcamera: controls: Simplify\n\tControlValue::{ControlValue, get, set}","Message-ID":"<20250401214715.GI3494@pendragon.ideasonboard.com>","References":"<20250401131939.749583-1-barnabas.pocze@ideasonboard.com>\n\t<20250401131939.749583-5-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250401131939.749583-5-barnabas.pocze@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33880,"web_url":"https://patchwork.libcamera.org/comment/33880/","msgid":"<149a8f08-43bc-489c-a0e6-f14fc307df50@ideasonboard.com>","date":"2025-04-02T08:39:08","subject":"Re: [PATCH v1 4/4] libcamera: controls: Simplify\n\tControlValue::{ControlValue, get, set}","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 04. 01. 23:47 keltezéssel, Laurent Pinchart írta:\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Tue, Apr 01, 2025 at 03:19:39PM +0200, Barnabás Pőcze wrote:\n>> Whether or not the control has an array type can be determined from the\n>> static information in `detail::control_type`, so use that and\n>> `if constexpr` to deduplicate the constructor, get, and set functions.\n> \n> I'd rather not do this, as it would prevent us from supporting arrays of\n> strings. Is there anything you're working on that depends on this\n> series, or is it standalone cleanups ?\n\nNothing depends on this change per se, but I have made use of the same uniform\nhandling of control values in other code. So I would like to achieve something\nmore uniform. Can we establish how multi-dimensional things are expected to work?\n\n\nRegards,\nBarnabás Pőcze\n\n> \n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> include/libcamera/controls.h | 75 ++++++++++--------------------------\n>> 1 file changed, 20 insertions(+), 55 deletions(-)\n>>\n>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>> index 85c724ec1..22b5e3d96 100644\n>> --- a/include/libcamera/controls.h\n>> +++ b/include/libcamera/controls.h\n>> @@ -136,28 +136,14 @@ public:\n>> \tControlValue();\n>> \n>> #ifndef __DOXYGEN__\n>> -\ttemplate::value &&\n>> -\t\t\t\t\t details::control_type::value &&\n>> -\t\t\t\t\t !std::is_same>::value,\n>> -\t\t\t\t\t std::nullptr_t> = nullptr>\n>> -\tControlValue(const T &value)\n>> -\t\t: type_(ControlTypeNone), numElements_(0)\n>> -\t{\n>> -\t\tset(details::control_type>::value, false,\n>> -\t\t &value, 1, sizeof(T));\n>> -\t}\n>> -\n>> -\ttemplate::value ||\n>> -\t\t\t\t\t std::is_same>::value,\n>> -\t\t\t\t\t std::nullptr_t> = nullptr>\n>> +\ttemplate::value)>>\n>> #else\n>> \ttemplate\n>> #endif\n>> \tControlValue(const T &value)\n>> -\t\t: type_(ControlTypeNone), numElements_(0)\n>> +\t\t: type_(ControlTypeNone), isArray_(false), numElements_(0)\n>> \t{\n>> -\t\tset(details::control_type>::value, true,\n>> -\t\t value.data(), value.size(), sizeof(typename T::value_type));\n>> +\t\tset(value);\n>> \t}\n>> \n>> \t~ControlValue();\n>> @@ -180,54 +166,33 @@ public:\n>> \t\treturn !(*this == other);\n>> \t}\n>> \n>> -#ifndef __DOXYGEN__\n>> -\ttemplate::value &&\n>> -\t\t\t\t\t !std::is_same>::value,\n>> -\t\t\t\t\t std::nullptr_t> = nullptr>\n>> -\tT get() const\n>> -\t{\n>> -\t\tassert(type_ == details::control_type>::value);\n>> -\t\tassert(!isArray_);\n>> -\n>> -\t\treturn *reinterpret_cast(data().data());\n>> -\t}\n>> -\n>> -\ttemplate::value ||\n>> -\t\t\t\t\t std::is_same>::value,\n>> -\t\t\t\t\t std::nullptr_t> = nullptr>\n>> -#else\n>> \ttemplate\n>> -#endif\n>> \tT get() const\n>> \t{\n>> -\t\tassert(type_ == details::control_type>::value);\n>> -\t\tassert(isArray_);\n>> +\t\tusing TypeInfo = details::control_type>;\n>> \n>> -\t\tusing V = typename T::value_type;\n>> -\t\tconst V *value = reinterpret_cast(data().data());\n>> -\t\treturn T{ value, numElements_ };\n>> -\t}\n>> +\t\tassert(type_ == TypeInfo::value);\n>> +\t\tassert(isArray_ == (TypeInfo::size > 0));\n>> \n>> -#ifndef __DOXYGEN__\n>> -\ttemplate::value &&\n>> -\t\t\t\t\t !std::is_same>::value,\n>> -\t\t\t\t\t std::nullptr_t> = nullptr>\n>> -\tvoid set(const T &value)\n>> -\t{\n>> -\t\tset(details::control_type>::value, false,\n>> -\t\t reinterpret_cast(&value), 1, sizeof(T));\n>> +\t\tif constexpr (TypeInfo::size > 0)\n>> +\t\t\treturn T(reinterpret_cast(data().data()), numElements_);\n>> +\t\telse\n>> +\t\t\treturn *reinterpret_cast(data().data());\n>> \t}\n>> \n>> -\ttemplate::value ||\n>> -\t\t\t\t\t std::is_same>::value,\n>> -\t\t\t\t\t std::nullptr_t> = nullptr>\n>> -#else\n>> \ttemplate\n>> -#endif\n>> \tvoid set(const T &value)\n>> \t{\n>> -\t\tset(details::control_type>::value, true,\n>> -\t\t value.data(), value.size(), sizeof(typename T::value_type));\n>> +\t\tusing TypeInfo = details::control_type>;\n>> +\t\tconstexpr auto type = TypeInfo::value;\n>> +\n>> +\t\tif constexpr (TypeInfo::size > 0) {\n>> +\t\t\tstatic_assert(std::is_trivially_copyable_v);\n>> +\t\t\tset(type, true, value.data(), value.size(), sizeof(typename T::value_type));\n>> +\t\t} else {\n>> +\t\t\tstatic_assert(std::is_trivially_copyable_v);\n>> +\t\t\tset(type, false, reinterpret_cast(&value), 1, sizeof(T));\n>> +\t\t}\n>> \t}\n>> \n>> \tvoid reserve(ControlType type, bool isArray = false,\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 F05AAC323E\n\tfor ;\n\tWed, 2 Apr 2025 08:39:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 09EBB6897A;\n\tWed, 2 Apr 2025 10:39:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 08FFA68979\n\tfor ;\n\tWed, 2 Apr 2025 10:39:13 +0200 (CEST)","from [192.168.33.20] (185.221.143.221.nat.pool.zt.hu\n\t[185.221.143.221])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3092B415;\n\tWed, 2 Apr 2025 10:37:20 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"O7Llmk05\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743583040;\n\tbh=xp+9bTg3xEy+WwpiXG/6LSIJ1xegU5GWIVvLIR40YZc=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=O7Llmk05ERPSA/q0Sy33jo7RNM/u2U7oAAhP6leNqbxH45UbIkNU8UrKw/zNAtarF\n\te8oeRI/VY+kTmQvS0N87oTqfPR2BoC4QWqIKueX0w91QasrXlW+ZcO6IA/SupP3aeE\n\tyS0Lk6DQdxmN4fadCC8kMLCk8Uh42yw3S5gvazJk=","Message-ID":"<149a8f08-43bc-489c-a0e6-f14fc307df50@ideasonboard.com>","Date":"Wed, 2 Apr 2025 10:39:08 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 4/4] libcamera: controls: Simplify\n\tControlValue::{ControlValue, get, set}","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250401131939.749583-1-barnabas.pocze@ideasonboard.com>\n\t<20250401131939.749583-5-barnabas.pocze@ideasonboard.com>\n\t<20250401214715.GI3494@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250401214715.GI3494@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]