[{"id":36560,"web_url":"https://patchwork.libcamera.org/comment/36560/","msgid":"<176184808662.3742839.5546703212677967486@ping.linuxembedded.co.uk>","date":"2025-10-30T18:14:46","subject":"Re: [PATCH v2 2/2] libcamera: base: utils: Simplify hex adaptor","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2025-10-30 18:11:27)\n> The libcamera hex string adaptor specifies and casts each type\n> specifically to map the size of each type.\n> \n> This needlessly repeats itself for each type and further more has a bug\n> with signed integer extension which causes values such as 0x80 to be\n> printed as 0xffffffffffffff80 instead.\n> \n> Remove the template specialisations for each type, and unify with a\n> single templated constructor of the struct hex trait.\n> \n\nAnd for the credit for the idea ;-)\n\nSuggested-by: Barnabás Pőcze \n\n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/base/utils.h | 66 ++++++----------------------------\n> src/libcamera/base/utils.cpp | 2 +-\n> test/meson.build | 2 +-\n> 3 files changed, 12 insertions(+), 58 deletions(-)\n> \n> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> index d32bd1cd62e0..3b3da7b2a887 100644\n> --- a/include/libcamera/base/utils.h\n> +++ b/include/libcamera/base/utils.h\n> @@ -78,67 +78,21 @@ struct timespec duration_to_timespec(const duration &value);\n> std::string time_point_to_string(const time_point &time);\n> \n> #ifndef __DOXYGEN__\n> -struct _hex {\n> +struct hex {\n> uint64_t v;\n> unsigned int w;\n> +\n> + template::value> * = nullptr>\n> + hex(T value, unsigned int width = 0)\n> + : v(static_cast>(value)),\n> + w(width ? width : sizeof(T) * 2)\n> + { }\n> };\n> \n> std::basic_ostream> &\n> -operator<<(std::basic_ostream> &stream, const _hex &h);\n> -#endif\n> -\n> -template - std::enable_if_t::value> * = nullptr>\n> -_hex hex(T value, unsigned int width = 0);\n> -\n> -#ifndef __DOXYGEN__\n> -template<>\n> -inline _hex hex(int8_t value, unsigned int width)\n> -{\n> - return { static_cast(value), width ? width : 2 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(uint8_t value, unsigned int width)\n> -{\n> - return { static_cast(value), width ? width : 2 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(int16_t value, unsigned int width)\n> -{\n> - return { static_cast(value), width ? width : 4 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(uint16_t value, unsigned int width)\n> -{\n> - return { static_cast(value), width ? width : 4 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(int32_t value, unsigned int width)\n> -{\n> - return { static_cast(value), width ? width : 8 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(uint32_t value, unsigned int width)\n> -{\n> - return { static_cast(value), width ? width : 8 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(int64_t value, unsigned int width)\n> -{\n> - return { static_cast(value), width ? width : 16 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(uint64_t value, unsigned int width)\n> -{\n> - return { static_cast(value), width ? width : 16 };\n> -}\n> +operator<<(std::basic_ostream> &stream, const struct hex &h);\n> +#else\n> +void hex(T value, unsigned int width = 0);\n> #endif\n> \n> size_t strlcpy(char *dst, const char *src, size_t size);\n> diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> index cb9fe0049c83..db868be224a2 100644\n> --- a/src/libcamera/base/utils.cpp\n> +++ b/src/libcamera/base/utils.cpp\n> @@ -187,7 +187,7 @@ std::string time_point_to_string(const time_point &time)\n> }\n> \n> std::basic_ostream> &\n> -operator<<(std::basic_ostream> &stream, const _hex &h)\n> +operator<<(std::basic_ostream> &stream, const struct hex &h)\n> {\n> stream << \"0x\";\n> \n> diff --git a/test/meson.build b/test/meson.build\n> index 96c4477f04b2..52f04364e4fc 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -73,7 +73,7 @@ internal_tests = [\n> {'name': 'timer-fail', 'sources': ['timer-fail.cpp'], 'should_fail': true},\n> {'name': 'timer-thread', 'sources': ['timer-thread.cpp']},\n> {'name': 'unique-fd', 'sources': ['unique-fd.cpp']},\n> - {'name': 'utils', 'sources': ['utils.cpp'], 'should_fail': true},\n> + {'name': 'utils', 'sources': ['utils.cpp']},\n> {'name': 'vector', 'sources': ['vector.cpp']},\n> {'name': 'yaml-parser', 'sources': ['yaml-parser.cpp']},\n> ]\n> -- \n> 2.50.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 932F9BE080\n\tfor ;\n\tThu, 30 Oct 2025 18:14:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C7B30608F5;\n\tThu, 30 Oct 2025 19:14:51 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A10E608CF\n\tfor ;\n\tThu, 30 Oct 2025 19:14:50 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1288018E5\n\tfor ;\n\tThu, 30 Oct 2025 19:13:00 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lbC3Aj/0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761847980;\n\tbh=AvTwza1wg6gqyecZCM3bx/B82KeGDXYFPsVeVozxWeM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=lbC3Aj/013aSu5PaY7X9kA1ALxXcYx8H0LYzI0Zy+ifalJik1ppOXzkiKy+GCHEbT\n\trxroV8/ndcaEly6yt7CFJZhWmZH9M7vGn+gdKXi6WoGuN+YSVZbCNaxevXC1w15yQl\n\tNZAO4/o/JEsQTc5Qwq393p2bEVQtVrYoQqqtQh30=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251030181127.1059501-3-kieran.bingham@ideasonboard.com>","References":"<20251030181127.1059501-1-kieran.bingham@ideasonboard.com>\n\t<20251030181127.1059501-3-kieran.bingham@ideasonboard.com>","Subject":"Re: [PATCH v2 2/2] libcamera: base: utils: Simplify hex adaptor","From":"Kieran Bingham ","Cc":"","To":"libcamera devel ","Date":"Thu, 30 Oct 2025 18:14:46 +0000","Message-ID":"<176184808662.3742839.5546703212677967486@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":36565,"web_url":"https://patchwork.libcamera.org/comment/36565/","msgid":"<468e4a8c-7d72-4f47-835b-9fa42a26c7a6@ideasonboard.com>","date":"2025-10-31T09:04:55","subject":"Re: [PATCH v2 2/2] libcamera: base: utils: Simplify hex adaptor","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. 10. 30. 19:11 keltezéssel, Kieran Bingham írta:\n> The libcamera hex string adaptor specifies and casts each type\n> specifically to map the size of each type.\n> \n> This needlessly repeats itself for each type and further more has a bug\n> with signed integer extension which causes values such as 0x80 to be\n> printed as 0xffffffffffffff80 instead.\n> \n> Remove the template specialisations for each type, and unify with a\n> single templated constructor of the struct hex trait.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/base/utils.h | 66 ++++++----------------------------\n> src/libcamera/base/utils.cpp | 2 +-\n> test/meson.build | 2 +-\n> 3 files changed, 12 insertions(+), 58 deletions(-)\n> \n> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> index d32bd1cd62e0..3b3da7b2a887 100644\n> --- a/include/libcamera/base/utils.h\n> +++ b/include/libcamera/base/utils.h\n> @@ -78,67 +78,21 @@ struct timespec duration_to_timespec(const duration &value);\n> std::string time_point_to_string(const time_point &time);\n> \n> #ifndef __DOXYGEN__\n> -struct _hex {\n> +struct hex {\n> \tuint64_t v;\n> \tunsigned int w;\n> +\n> +\ttemplate::value> * = nullptr>\n\nThis can be `std::is_integral_v`.\n\n\n> +\thex(T value, unsigned int width = 0)\n\nBetter idea... `width = sizeof(T) * 2`\n\n\n> +\t: v(static_cast>(value)),\n> +\t w(width ? width : sizeof(T) * 2)\n\n... then this can be just `w(width)`\n\nAnd I think usually the member init list has an extra level of indentation.\n\n> +\t{ }\n> };\n> \n> std::basic_ostream> &\n> -operator<<(std::basic_ostream> &stream, const _hex &h);\n> -#endif\n> -\n> -template -\t std::enable_if_t::value> * = nullptr>\n> -_hex hex(T value, unsigned int width = 0);\n> -\n> -#ifndef __DOXYGEN__\n> -template<>\n> -inline _hex hex(int8_t value, unsigned int width)\n> -{\n> -\treturn { static_cast(value), width ? width : 2 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(uint8_t value, unsigned int width)\n> -{\n> -\treturn { static_cast(value), width ? width : 2 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(int16_t value, unsigned int width)\n> -{\n> -\treturn { static_cast(value), width ? width : 4 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(uint16_t value, unsigned int width)\n> -{\n> -\treturn { static_cast(value), width ? width : 4 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(int32_t value, unsigned int width)\n> -{\n> -\treturn { static_cast(value), width ? width : 8 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(uint32_t value, unsigned int width)\n> -{\n> -\treturn { static_cast(value), width ? width : 8 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(int64_t value, unsigned int width)\n> -{\n> -\treturn { static_cast(value), width ? width : 16 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(uint64_t value, unsigned int width)\n> -{\n> -\treturn { static_cast(value), width ? width : 16 };\n> -}\n> +operator<<(std::basic_ostream> &stream, const struct hex &h);\n\nDo you need the `struct hex` here?\n\n\n> +#else\n> +void hex(T value, unsigned int width = 0);\n\nI suppose this is to appease doxygen? Does it not need a `template`?\n\n\nRegards,\nBarnabás Pőcze\n\n> #endif\n> \n> size_t strlcpy(char *dst, const char *src, size_t size);\n> diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> index cb9fe0049c83..db868be224a2 100644\n> --- a/src/libcamera/base/utils.cpp\n> +++ b/src/libcamera/base/utils.cpp\n> @@ -187,7 +187,7 @@ std::string time_point_to_string(const time_point &time)\n> }\n> \n> std::basic_ostream> &\n> -operator<<(std::basic_ostream> &stream, const _hex &h)\n> +operator<<(std::basic_ostream> &stream, const struct hex &h)\n> {\n> \tstream << \"0x\";\n> \n> diff --git a/test/meson.build b/test/meson.build\n> index 96c4477f04b2..52f04364e4fc 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -73,7 +73,7 @@ internal_tests = [\n> {'name': 'timer-fail', 'sources': ['timer-fail.cpp'], 'should_fail': true},\n> {'name': 'timer-thread', 'sources': ['timer-thread.cpp']},\n> {'name': 'unique-fd', 'sources': ['unique-fd.cpp']},\n> - {'name': 'utils', 'sources': ['utils.cpp'], 'should_fail': true},\n> + {'name': 'utils', 'sources': ['utils.cpp']},\n> {'name': 'vector', 'sources': ['vector.cpp']},\n> {'name': 'yaml-parser', 'sources': ['yaml-parser.cpp']},\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 E99EABE080\n\tfor ;\n\tFri, 31 Oct 2025 09:05:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8F0BD60971;\n\tFri, 31 Oct 2025 10:05:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE665606E6\n\tfor ;\n\tFri, 31 Oct 2025 10:04:58 +0100 (CET)","from [192.168.33.30] (185.221.140.239.nat.pool.zt.hu\n\t[185.221.140.239])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 58786664;\n\tFri, 31 Oct 2025 10:03:08 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"u3mnqo4m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761901388;\n\tbh=RS32NM7aHmOXEEqUq0sOrmsXtIOML/vc4AfKw8J7wTQ=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=u3mnqo4mOOTBzP52fmmHPm6R7b/IxhDO5Zzd96IyEYHcVHQEpWrT2BWylBV6IJ7p0\n\t3JVeIPXEJcHpPB3FjXTuZhsb630hVVN905Y2RItW+G3QyQChEBYGJjUSrscDEgPLcs\n\tMQGef1MkO0OD60nXwJ4SKF1FRZvhZK9L1D2n4mtM=","Message-ID":"<468e4a8c-7d72-4f47-835b-9fa42a26c7a6@ideasonboard.com>","Date":"Fri, 31 Oct 2025 10:04:55 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 2/2] libcamera: base: utils: Simplify hex adaptor","To":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20251030181127.1059501-1-kieran.bingham@ideasonboard.com>\n\t<20251030181127.1059501-3-kieran.bingham@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20251030181127.1059501-3-kieran.bingham@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\" "}},{"id":36566,"web_url":"https://patchwork.libcamera.org/comment/36566/","msgid":"<20251031090750.GG797@pendragon.ideasonboard.com>","date":"2025-10-31T09:07:50","subject":"Re: [PATCH v2 2/2] libcamera: base: utils: Simplify hex adaptor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Oct 30, 2025 at 06:11:27PM +0000, Kieran Bingham wrote:\n> The libcamera hex string adaptor specifies and casts each type\n> specifically to map the size of each type.\n> \n> This needlessly repeats itself for each type and further more has a bug\n> with signed integer extension which causes values such as 0x80 to be\n> printed as 0xffffffffffffff80 instead.\n> \n> Remove the template specialisations for each type, and unify with a\n> single templated constructor of the struct hex trait.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/base/utils.h | 66 ++++++----------------------------\n> src/libcamera/base/utils.cpp | 2 +-\n> test/meson.build | 2 +-\n> 3 files changed, 12 insertions(+), 58 deletions(-)\n> \n> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> index d32bd1cd62e0..3b3da7b2a887 100644\n> --- a/include/libcamera/base/utils.h\n> +++ b/include/libcamera/base/utils.h\n> @@ -78,67 +78,21 @@ struct timespec duration_to_timespec(const duration &value);\n> std::string time_point_to_string(const time_point &time);\n> \n> #ifndef __DOXYGEN__\n> -struct _hex {\n> +struct hex {\n> \tuint64_t v;\n> \tunsigned int w;\n> +\n> +\ttemplate::value> * = nullptr>\n> +\thex(T value, unsigned int width = 0)\n> +\t: v(static_cast>(value)),\n> +\t w(width ? width : sizeof(T) * 2)\n> +\t{ }\n\n\ttemplate::value> * = nullptr>\n\thex(T value, unsigned int width = 0)\n\t\t: v(static_cast>(value)),\n\t\t w(width ? width : sizeof(T) * 2)\n\t{\n\t}\n\n(our coding style indents the initializers)\n\n> };\n> \n> std::basic_ostream> &\n> -operator<<(std::basic_ostream> &stream, const _hex &h);\n> -#endif\n> -\n> -template -\t std::enable_if_t::value> * = nullptr>\n> -_hex hex(T value, unsigned int width = 0);\n> -\n> -#ifndef __DOXYGEN__\n> -template<>\n> -inline _hex hex(int8_t value, unsigned int width)\n> -{\n> -\treturn { static_cast(value), width ? width : 2 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(uint8_t value, unsigned int width)\n> -{\n> -\treturn { static_cast(value), width ? width : 2 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(int16_t value, unsigned int width)\n> -{\n> -\treturn { static_cast(value), width ? width : 4 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(uint16_t value, unsigned int width)\n> -{\n> -\treturn { static_cast(value), width ? width : 4 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(int32_t value, unsigned int width)\n> -{\n> -\treturn { static_cast(value), width ? width : 8 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(uint32_t value, unsigned int width)\n> -{\n> -\treturn { static_cast(value), width ? width : 8 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(int64_t value, unsigned int width)\n> -{\n> -\treturn { static_cast(value), width ? width : 16 };\n> -}\n> -\n> -template<>\n> -inline _hex hex(uint64_t value, unsigned int width)\n> -{\n> -\treturn { static_cast(value), width ? width : 16 };\n> -}\n> +operator<<(std::basic_ostream> &stream, const struct hex &h);\n> +#else\n> +void hex(T value, unsigned int width = 0);\n> #endif\n> \n> size_t strlcpy(char *dst, const char *src, size_t size);\n> diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> index cb9fe0049c83..db868be224a2 100644\n> --- a/src/libcamera/base/utils.cpp\n> +++ b/src/libcamera/base/utils.cpp\n> @@ -187,7 +187,7 @@ std::string time_point_to_string(const time_point &time)\n> }\n> \n> std::basic_ostream> &\n> -operator<<(std::basic_ostream> &stream, const _hex &h)\n> +operator<<(std::basic_ostream> &stream, const struct hex &h)\n\nAny specific reason for adding the \"struct\" keyword here (and in the .h\nfile) ?\n\n> {\n> \tstream << \"0x\";\n> \n> diff --git a/test/meson.build b/test/meson.build\n> index 96c4477f04b2..52f04364e4fc 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -73,7 +73,7 @@ internal_tests = [\n> {'name': 'timer-fail', 'sources': ['timer-fail.cpp'], 'should_fail': true},\n> {'name': 'timer-thread', 'sources': ['timer-thread.cpp']},\n> {'name': 'unique-fd', 'sources': ['unique-fd.cpp']},\n> - {'name': 'utils', 'sources': ['utils.cpp'], 'should_fail': true},\n> + {'name': 'utils', 'sources': ['utils.cpp']},\n> {'name': 'vector', 'sources': ['vector.cpp']},\n> {'name': 'yaml-parser', 'sources': ['yaml-parser.cpp']},\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 C3BEEC3259\n\tfor ;\n\tFri, 31 Oct 2025 09:08:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1B4860970;\n\tFri, 31 Oct 2025 10:08:05 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4FE15606E6\n\tfor ;\n\tFri, 31 Oct 2025 10:08:04 +0100 (CET)","from pendragon.ideasonboard.com (unknown [193.209.96.36])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id B133C15D2; \n\tFri, 31 Oct 2025 10:06:13 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Fv2jnIQf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761901573;\n\tbh=Sc17lRYml3f+WJeO0v29vriJT3YbQ1OSKqH0qSTTiRQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Fv2jnIQfV8JD388Sg2UC9kBTsIs6e53/Myg3WIPp4aOBjMzbdm832UBlpEZq0Cer2\n\tb93eQKuG6hnIUISfBmjMMjuU8PoBwjdFqQZMqGVQOgl4m7Z45q9eqAZU1HvVg9NhNL\n\tvvN7n8EkTT7TDjdU3wIEX5nYCSjaYVwuboM+LntA=","Date":"Fri, 31 Oct 2025 11:07:50 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera devel ","Subject":"Re: [PATCH v2 2/2] libcamera: base: utils: Simplify hex adaptor","Message-ID":"<20251031090750.GG797@pendragon.ideasonboard.com>","References":"<20251030181127.1059501-1-kieran.bingham@ideasonboard.com>\n\t<20251030181127.1059501-3-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251030181127.1059501-3-kieran.bingham@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":36569,"web_url":"https://patchwork.libcamera.org/comment/36569/","msgid":"<176190752991.567526.12205486107468163704@ping.linuxembedded.co.uk>","date":"2025-10-31T10:45:29","subject":"Re: [PATCH v2 2/2] libcamera: base: utils: Simplify hex adaptor","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-10-31 09:04:55)\n> Hi\n> \n> \n> 2025. 10. 30. 19:11 keltezéssel, Kieran Bingham írta:\n> > The libcamera hex string adaptor specifies and casts each type\n> > specifically to map the size of each type.\n> > \n> > This needlessly repeats itself for each type and further more has a bug\n> > with signed integer extension which causes values such as 0x80 to be\n> > printed as 0xffffffffffffff80 instead.\n> > \n> > Remove the template specialisations for each type, and unify with a\n> > single templated constructor of the struct hex trait.\n> > \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > include/libcamera/base/utils.h | 66 ++++++----------------------------\n> > src/libcamera/base/utils.cpp | 2 +-\n> > test/meson.build | 2 +-\n> > 3 files changed, 12 insertions(+), 58 deletions(-)\n> > \n> > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> > index d32bd1cd62e0..3b3da7b2a887 100644\n> > --- a/include/libcamera/base/utils.h\n> > +++ b/include/libcamera/base/utils.h\n> > @@ -78,67 +78,21 @@ struct timespec duration_to_timespec(const duration &value);\n> > std::string time_point_to_string(const time_point &time);\n> > \n> > #ifndef __DOXYGEN__\n> > -struct _hex {\n> > +struct hex {\n> > uint64_t v;\n> > unsigned int w;\n> > +\n> > + template::value> * = nullptr>\n> \n> This can be `std::is_integral_v`.\n> \n\nack\n\n> \n> > + hex(T value, unsigned int width = 0)\n> \n> Better idea... `width = sizeof(T) * 2`\n> \n> \n> > + : v(static_cast>(value)),\n> > + w(width ? width : sizeof(T) * 2)\n> \n> ... then this can be just `w(width)`\n> \n\nAh yes that's an nicer solution.\n\n> And I think usually the member init list has an extra level of indentation.\n> \n\nack.\n\n> > + { }\n> > };\n> > \n> > std::basic_ostream> &\n> > -operator<<(std::basic_ostream> &stream, const _hex &h);\n> > -#endif\n> > -\n> > -template > - std::enable_if_t::value> * = nullptr>\n> > -_hex hex(T value, unsigned int width = 0);\n> > -\n> > -#ifndef __DOXYGEN__\n> > -template<>\n> > -inline _hex hex(int8_t value, unsigned int width)\n> > -{\n> > - return { static_cast(value), width ? width : 2 };\n> > -}\n> > -\n> > -template<>\n> > -inline _hex hex(uint8_t value, unsigned int width)\n> > -{\n> > - return { static_cast(value), width ? width : 2 };\n> > -}\n> > -\n> > -template<>\n> > -inline _hex hex(int16_t value, unsigned int width)\n> > -{\n> > - return { static_cast(value), width ? width : 4 };\n> > -}\n> > -\n> > -template<>\n> > -inline _hex hex(uint16_t value, unsigned int width)\n> > -{\n> > - return { static_cast(value), width ? width : 4 };\n> > -}\n> > -\n> > -template<>\n> > -inline _hex hex(int32_t value, unsigned int width)\n> > -{\n> > - return { static_cast(value), width ? width : 8 };\n> > -}\n> > -\n> > -template<>\n> > -inline _hex hex(uint32_t value, unsigned int width)\n> > -{\n> > - return { static_cast(value), width ? width : 8 };\n> > -}\n> > -\n> > -template<>\n> > -inline _hex hex(int64_t value, unsigned int width)\n> > -{\n> > - return { static_cast(value), width ? width : 16 };\n> > -}\n> > -\n> > -template<>\n> > -inline _hex hex(uint64_t value, unsigned int width)\n> > -{\n> > - return { static_cast(value), width ? width : 16 };\n> > -}\n> > +operator<<(std::basic_ostream> &stream, const struct hex &h);\n> \n> Do you need the `struct hex` here?\n\nReading Laurent's comment I assume you mean the keyword struct. I guess\nnot - I added it because it's now straight to the struct hex, but I\nguess previously it was struct _hex and worked fine ;D\n\n> > +#else\n> > +void hex(T value, unsigned int width = 0);\n> \n> I suppose this is to appease doxygen? Does it not need a `template`?\n\nYes, it was to make doxygen be happy. It hasn't complained about\ntemplate - I'll check again how it builds and adapt / add if needed.\n\n> Regards,\n> Barnabás Pőcze\n\nThanks\n\n> \n> > #endif\n> > \n> > size_t strlcpy(char *dst, const char *src, size_t size);\n> > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> > index cb9fe0049c83..db868be224a2 100644\n> > --- a/src/libcamera/base/utils.cpp\n> > +++ b/src/libcamera/base/utils.cpp\n> > @@ -187,7 +187,7 @@ std::string time_point_to_string(const time_point &time)\n> > }\n> > \n> > std::basic_ostream> &\n> > -operator<<(std::basic_ostream> &stream, const _hex &h)\n> > +operator<<(std::basic_ostream> &stream, const struct hex &h)\n> > {\n> > stream << \"0x\";\n> > \n> > diff --git a/test/meson.build b/test/meson.build\n> > index 96c4477f04b2..52f04364e4fc 100644\n> > --- a/test/meson.build\n> > +++ b/test/meson.build\n> > @@ -73,7 +73,7 @@ internal_tests = [\n> > {'name': 'timer-fail', 'sources': ['timer-fail.cpp'], 'should_fail': true},\n> > {'name': 'timer-thread', 'sources': ['timer-thread.cpp']},\n> > {'name': 'unique-fd', 'sources': ['unique-fd.cpp']},\n> > - {'name': 'utils', 'sources': ['utils.cpp'], 'should_fail': true},\n> > + {'name': 'utils', 'sources': ['utils.cpp']},\n> > {'name': 'vector', 'sources': ['vector.cpp']},\n> > {'name': 'yaml-parser', 'sources': ['yaml-parser.cpp']},\n> > ]\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 26803BE080\n\tfor ;\n\tFri, 31 Oct 2025 10:45:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F3A576097B;\n\tFri, 31 Oct 2025 11:45:35 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9F592606E6\n\tfor ;\n\tFri, 31 Oct 2025 11:45:33 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B01B614B0;\n\tFri, 31 Oct 2025 11:43:42 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"fJMiyu97\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761907422;\n\tbh=Ziz0hgsSIU/unXlWEGDDpjMMKs2L2SfhVn0YCMNELBo=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=fJMiyu9774TJRxVQ/PniG4vVLiyaF24TKKe+vPy6or5ohXLfmrWBWFb4aqTwT0nWz\n\tqChok2NY8GpMEm7aydjWKg52h1R3ZaNQuqghSbwzy1rwSEq2KcTWrXk7Uynyp5Su6y\n\trZv10+ijf4FmtqvqRtpI1ZIEzltY+m6y1g+tsRDs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<468e4a8c-7d72-4f47-835b-9fa42a26c7a6@ideasonboard.com>","References":"<20251030181127.1059501-1-kieran.bingham@ideasonboard.com>\n\t<20251030181127.1059501-3-kieran.bingham@ideasonboard.com>\n\t<468e4a8c-7d72-4f47-835b-9fa42a26c7a6@ideasonboard.com>","Subject":"Re: [PATCH v2 2/2] libcamera: base: utils: Simplify hex adaptor","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera devel ","Date":"Fri, 31 Oct 2025 10:45:29 +0000","Message-ID":"<176190752991.567526.12205486107468163704@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":36570,"web_url":"https://patchwork.libcamera.org/comment/36570/","msgid":"<176190765297.567526.3563465308955386231@ping.linuxembedded.co.uk>","date":"2025-10-31T10:47:32","subject":"Re: [PATCH v2 2/2] libcamera: base: utils: Simplify hex adaptor","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2025-10-31 09:07:50)\n> On Thu, Oct 30, 2025 at 06:11:27PM +0000, Kieran Bingham wrote:\n> > The libcamera hex string adaptor specifies and casts each type\n> > specifically to map the size of each type.\n> > \n> > This needlessly repeats itself for each type and further more has a bug\n> > with signed integer extension which causes values such as 0x80 to be\n> > printed as 0xffffffffffffff80 instead.\n> > \n> > Remove the template specialisations for each type, and unify with a\n> > single templated constructor of the struct hex trait.\n> > \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > include/libcamera/base/utils.h | 66 ++++++----------------------------\n> > src/libcamera/base/utils.cpp | 2 +-\n> > test/meson.build | 2 +-\n> > 3 files changed, 12 insertions(+), 58 deletions(-)\n> > \n> > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> > index d32bd1cd62e0..3b3da7b2a887 100644\n> > --- a/include/libcamera/base/utils.h\n> > +++ b/include/libcamera/base/utils.h\n> > @@ -78,67 +78,21 @@ struct timespec duration_to_timespec(const duration &value);\n> > std::string time_point_to_string(const time_point &time);\n> > \n> > #ifndef __DOXYGEN__\n> > -struct _hex {\n> > +struct hex {\n> > uint64_t v;\n> > unsigned int w;\n> > +\n> > + template::value> * = nullptr>\n> > + hex(T value, unsigned int width = 0)\n> > + : v(static_cast>(value)),\n> > + w(width ? width : sizeof(T) * 2)\n> > + { }\n> \n> template::value> * = nullptr>\n> hex(T value, unsigned int width = 0)\n> : v(static_cast>(value)),\n> w(width ? width : sizeof(T) * 2)\n> {\n> }\n> \n> (our coding style indents the initializers)\n\nI'll update along with Barnabas' suggestions.\n\n> \n> > };\n> > \n> > std::basic_ostream> &\n> > -operator<<(std::basic_ostream> &stream, const _hex &h);\n> > -#endif\n> > -\n> > -template > - std::enable_if_t::value> * = nullptr>\n> > -_hex hex(T value, unsigned int width = 0);\n> > -\n> > -#ifndef __DOXYGEN__\n> > -template<>\n> > -inline _hex hex(int8_t value, unsigned int width)\n> > -{\n> > - return { static_cast(value), width ? width : 2 };\n> > -}\n> > -\n> > -template<>\n> > -inline _hex hex(uint8_t value, unsigned int width)\n> > -{\n> > - return { static_cast(value), width ? width : 2 };\n> > -}\n> > -\n> > -template<>\n> > -inline _hex hex(int16_t value, unsigned int width)\n> > -{\n> > - return { static_cast(value), width ? width : 4 };\n> > -}\n> > -\n> > -template<>\n> > -inline _hex hex(uint16_t value, unsigned int width)\n> > -{\n> > - return { static_cast(value), width ? width : 4 };\n> > -}\n> > -\n> > -template<>\n> > -inline _hex hex(int32_t value, unsigned int width)\n> > -{\n> > - return { static_cast(value), width ? width : 8 };\n> > -}\n> > -\n> > -template<>\n> > -inline _hex hex(uint32_t value, unsigned int width)\n> > -{\n> > - return { static_cast(value), width ? width : 8 };\n> > -}\n> > -\n> > -template<>\n> > -inline _hex hex(int64_t value, unsigned int width)\n> > -{\n> > - return { static_cast(value), width ? width : 16 };\n> > -}\n> > -\n> > -template<>\n> > -inline _hex hex(uint64_t value, unsigned int width)\n> > -{\n> > - return { static_cast(value), width ? width : 16 };\n> > -}\n> > +operator<<(std::basic_ostream> &stream, const struct hex &h);\n> > +#else\n> > +void hex(T value, unsigned int width = 0);\n> > #endif\n> > \n> > size_t strlcpy(char *dst, const char *src, size_t size);\n> > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> > index cb9fe0049c83..db868be224a2 100644\n> > --- a/src/libcamera/base/utils.cpp\n> > +++ b/src/libcamera/base/utils.cpp\n> > @@ -187,7 +187,7 @@ std::string time_point_to_string(const time_point &time)\n> > }\n> > \n> > std::basic_ostream> &\n> > -operator<<(std::basic_ostream> &stream, const _hex &h)\n> > +operator<<(std::basic_ostream> &stream, const struct hex &h)\n> \n> Any specific reason for adding the \"struct\" keyword here (and in the .h\n> file) ?\n> \n\nProbably because I thought I had to because I was 'changing the type'\n... but given _hex was also a struct ... I think it might work without\n:D\n\nI'll remove and retest for v3.\n\n> > {\n> > stream << \"0x\";\n> > \n> > diff --git a/test/meson.build b/test/meson.build\n> > index 96c4477f04b2..52f04364e4fc 100644\n> > --- a/test/meson.build\n> > +++ b/test/meson.build\n> > @@ -73,7 +73,7 @@ internal_tests = [\n> > {'name': 'timer-fail', 'sources': ['timer-fail.cpp'], 'should_fail': true},\n> > {'name': 'timer-thread', 'sources': ['timer-thread.cpp']},\n> > {'name': 'unique-fd', 'sources': ['unique-fd.cpp']},\n> > - {'name': 'utils', 'sources': ['utils.cpp'], 'should_fail': true},\n> > + {'name': 'utils', 'sources': ['utils.cpp']},\n> > {'name': 'vector', 'sources': ['vector.cpp']},\n> > {'name': 'yaml-parser', 'sources': ['yaml-parser.cpp']},\n> > ]\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 4C73BBE080\n\tfor ;\n\tFri, 31 Oct 2025 10:47:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 708426098A;\n\tFri, 31 Oct 2025 11:47:37 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9CEA2606E6\n\tfor ;\n\tFri, 31 Oct 2025 11:47:35 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E955B153F;\n\tFri, 31 Oct 2025 11:45:44 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"TkqfyAK5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761907545;\n\tbh=XQQfp4v9SdlzFoCJczOJOoz9spNOswIwdmrVwM9KAQQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=TkqfyAK5gw+IgxF/j7DNkA/ELIuJjxUG7E1XC0tX6SiRqt+Njf+w8hkkRxcWFDkF7\n\tIrryUY6zsCngk4sEq5jq2Hgp555xY770I4UAsWqmvZO5wJgfFYUdJKZCV8Wn2CCtt/\n\tq7OA/VF1Zt4TP4HaTk7y+dg7LO0W6nrGCeIQgBjM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251031090750.GG797@pendragon.ideasonboard.com>","References":"<20251030181127.1059501-1-kieran.bingham@ideasonboard.com>\n\t<20251030181127.1059501-3-kieran.bingham@ideasonboard.com>\n\t<20251031090750.GG797@pendragon.ideasonboard.com>","Subject":"Re: [PATCH v2 2/2] libcamera: base: utils: Simplify hex adaptor","From":"Kieran Bingham ","Cc":"libcamera devel ","To":"Laurent Pinchart ","Date":"Fri, 31 Oct 2025 10:47:32 +0000","Message-ID":"<176190765297.567526.3563465308955386231@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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\" "}}]