[{"id":36575,"web_url":"https://patchwork.libcamera.org/comment/36575/","msgid":"<0c1232c4-b41d-4894-a041-bef6ef839b81@ideasonboard.com>","date":"2025-10-31T11:59:43","subject":"Re: [PATCH v3 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\n2025. 10. 31. 12:52 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> Suggested-by: Barnabás Pőcze \n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/base/utils.h | 68 ++++++----------------------------\n> src/libcamera/base/utils.cpp | 2 +-\n> test/meson.build | 2 +-\n> 3 files changed, 14 insertions(+), 58 deletions(-)\n> \n> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> index d32bd1cd62e0..555da71f124b 100644\n> --- a/include/libcamera/base/utils.h\n> +++ b/include/libcamera/base/utils.h\n> @@ -78,67 +78,23 @@ 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> * = nullptr>\n> +\thex(T value, unsigned int width = sizeof(T) * 2)\n> +\t\t: v(static_cast>(value)),\n> +\t\t w(width)\n> +\t{\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 hex &h);\n> +#else\n> +template> * = nullptr>\n> +void hex(T value, unsigned int width = 0);\n> #endif\n\nI think maybe a question is whether to document the class and its\nconstructor or inject this function into the documentation.\n\nBut the rest looks good to me.\n\nReviewed-by: Barnabás Pőcze \n\n\n\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..446c9a05e96d 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 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 1EF1EBE080\n\tfor ;\n\tFri, 31 Oct 2025 11:59:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B5356098A;\n\tFri, 31 Oct 2025 12:59:48 +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 513FF60947\n\tfor ;\n\tFri, 31 Oct 2025 12:59:47 +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 82B881691;\n\tFri, 31 Oct 2025 12:57:56 +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=\"Mq01kGZ5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761911876;\n\tbh=KXc/IlA98O5r47L7LWDwUFGq4uQB/5DGae7BN2XlI9c=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Mq01kGZ5C90V8g9ua1vL54ygvPew9EZQOkYjIMxSWcz7nlkW1hFGCUMFEA2+xcFUx\n\tNzi0gEsfL/xCleSxwVBBn44basyTuvU3vZu8W7hLyf+AAp2K1vFWotFyGpF7vpdazF\n\tuQ2C7027lp0K4kzZ6ZC2rYmyDtT990zeH/3uXfwk=","Message-ID":"<0c1232c4-b41d-4894-a041-bef6ef839b81@ideasonboard.com>","Date":"Fri, 31 Oct 2025 12:59:43 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 2/2] libcamera: base: utils: Simplify hex adaptor","To":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20251031115216.3776845-1-kieran.bingham@ideasonboard.com>\n\t<20251031115216.3776845-3-kieran.bingham@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20251031115216.3776845-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":36576,"web_url":"https://patchwork.libcamera.org/comment/36576/","msgid":"<20251031122235.GI797@pendragon.ideasonboard.com>","date":"2025-10-31T12:22:35","subject":"Re: [PATCH v3 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 Fri, Oct 31, 2025 at 12:59:43PM +0100, Barnabás Pőcze wrote:\n> 2025. 10. 31. 12:52 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> > Suggested-by: Barnabás Pőcze \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > include/libcamera/base/utils.h | 68 ++++++----------------------------\n> > src/libcamera/base/utils.cpp | 2 +-\n> > test/meson.build | 2 +-\n> > 3 files changed, 14 insertions(+), 58 deletions(-)\n> > \n> > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> > index d32bd1cd62e0..555da71f124b 100644\n> > --- a/include/libcamera/base/utils.h\n> > +++ b/include/libcamera/base/utils.h\n> > @@ -78,67 +78,23 @@ 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> * = nullptr>\n> > +\thex(T value, unsigned int width = sizeof(T) * 2)\n> > +\t\t: v(static_cast>(value)),\n> > +\t\t w(width)\n> > +\t{\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 hex &h);\n> > +#else\n> > +template> * = nullptr>\n> > +void hex(T value, unsigned int width = 0);\n> > #endif\n> \n> I think maybe a question is whether to document the class and its\n> constructor or inject this function into the documentation.\n> \n> But the rest looks good to me.\n> \n> Reviewed-by: Barnabás Pőcze \n\nThis is what we currently document:\n\n/**\n * \\fn hex(T value, unsigned int width)\n * \\brief Write an hexadecimal value to an output string\n * \\param value The value\n * \\param width The width\n *\n * Return an object of unspecified type such that, if \\a os is the name of an\n * output stream of type std::ostream, and T is an integer type, then the\n * expression\n *\n * \\code{.cpp}\n * os << utils::hex(value)\n * \\endcode\n *\n * will output the \\a value to the stream in hexadecimal form with the base\n * prefix and the filling character set to '0'. The field width is set to \\a\n * width if specified to a non-zero value, or to the native width of type T\n * otherwise. The \\a os stream configuration is not modified.\n */\n\nI prefer hiding the structure as it's really an implementation detail.\nIf we want to avoid cheating, we could do\n\nnamespace details {\n\nstruct hex {\n\tuint64_t v;\n\tunsigned int w;\n};\n\n} /* namespace details */\n\ntemplate> * = nullptr>\ndetails::_ex hex(T value, unsigned int width = sizeof(T) * 2)\n{\n\treturn details::hex{ static_cast>(value)), width };\n}\n\nstd::basic_ostream> &\noperator<<(std::basic_ostream> &stream, const details::hex &h);\n\nNo need for #ifndef __DOXYGEN__ magic anymore, hex() stays a function\ncompatible with its current documentation, and details::hex is hidden\nautomatically as it's in a details namespace.\n\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..446c9a05e96d 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 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> > ]\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 78B66C3259\n\tfor ;\n\tFri, 31 Oct 2025 12:22:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 80AAB609C0;\n\tFri, 31 Oct 2025 13:22:52 +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 37AC160947\n\tfor ;\n\tFri, 31 Oct 2025 13:22:50 +0100 (CET)","from pendragon.ideasonboard.com (unknown [193.209.96.36])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 68034153F; \n\tFri, 31 Oct 2025 13:20:59 +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=\"rYxoxcO5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761913259;\n\tbh=obmo+jir2pJCJqR7iUn839dZ21aCiSfvkAPS1xWH558=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rYxoxcO5Aj9Jx7ShydqRYRc8ggQjSPuTLGPc5zYhkDdNipSh0QIH1kqGq/Yqhrhk1\n\t4GV/F1cf4jcXIiItYTWBYH1yAu3Bndtnmi+67DaJfhLARXcpLyXM/Rfb3wU8hEEZQa\n\tsQKSco4RRXqfWvNcLyCD4aMm38wI6t/oUhVIy2Y8=","Date":"Fri, 31 Oct 2025 14:22:35 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Kieran Bingham ,\n\tlibcamera devel ","Subject":"Re: [PATCH v3 2/2] libcamera: base: utils: Simplify hex adaptor","Message-ID":"<20251031122235.GI797@pendragon.ideasonboard.com>","References":"<20251031115216.3776845-1-kieran.bingham@ideasonboard.com>\n\t<20251031115216.3776845-3-kieran.bingham@ideasonboard.com>\n\t<0c1232c4-b41d-4894-a041-bef6ef839b81@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<0c1232c4-b41d-4894-a041-bef6ef839b81@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":36577,"web_url":"https://patchwork.libcamera.org/comment/36577/","msgid":"<176191349611.3770990.3036548183364784344@ping.linuxembedded.co.uk>","date":"2025-10-31T12:24:56","subject":"Re: [PATCH v3 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 11:59:43)\n> Hi\n> \n> 2025. 10. 31. 12:52 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> > Suggested-by: Barnabás Pőcze \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > include/libcamera/base/utils.h | 68 ++++++----------------------------\n> > src/libcamera/base/utils.cpp | 2 +-\n> > test/meson.build | 2 +-\n> > 3 files changed, 14 insertions(+), 58 deletions(-)\n> > \n> > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> > index d32bd1cd62e0..555da71f124b 100644\n> > --- a/include/libcamera/base/utils.h\n> > +++ b/include/libcamera/base/utils.h\n> > @@ -78,67 +78,23 @@ 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> * = nullptr>\n> > + hex(T value, unsigned int width = sizeof(T) * 2)\n> > + : v(static_cast>(value)),\n> > + w(width)\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 hex &h);\n> > +#else\n> > +template> * = nullptr>\n> > +void hex(T value, unsigned int width = 0);\n> > #endif\n> \n> I think maybe a question is whether to document the class and its\n> constructor or inject this function into the documentation.\n\nIt would require us documenting w and v ... and I think that's internal\nimplementation detail - I was happy with how the documentation was\nphrased already - so this just keeps that alignment.\n\nIt's the equivalent of these lines which were removed, except because\nnow it's a constructor it's void ... or just no return at all.\n\n> > -template > - std::enable_if_t::value> * = nullptr>\n> > -_hex hex(T value, unsigned int width = 0);\n\n--\nKieran\n\n\n> \n> But the rest looks good to me.\n> \n> Reviewed-by: Barnabás Pőcze \n> \n> \n> \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..446c9a05e96d 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 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 A7E5CC3259\n\tfor ;\n\tFri, 31 Oct 2025 12:25:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C85AF609D7;\n\tFri, 31 Oct 2025 13:25: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 44B9A60947\n\tfor ;\n\tFri, 31 Oct 2025 13:24:59 +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 4C7B0153F;\n\tFri, 31 Oct 2025 13:23: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=\"KFSF1g9n\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761913388;\n\tbh=JecTo+tuAehOTLcDsaH19+++604jxkC+lQa4x/NvVFw=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=KFSF1g9nmSM0kkn/EZOHP07uR3WwjDEnDjgT9HAYAoXS33+DL/hPaaOIQzh4fR79z\n\turMRkxNdBiyH1BbXyO6xe4gr3YroKRgRlt7gZDa5Ze8Tuai8ZvfTTXEBuomLB06BBx\n\tBWwFvz6R45avwmJXmWxf/HZSBS2oI1m/deTOMPws=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<0c1232c4-b41d-4894-a041-bef6ef839b81@ideasonboard.com>","References":"<20251031115216.3776845-1-kieran.bingham@ideasonboard.com>\n\t<20251031115216.3776845-3-kieran.bingham@ideasonboard.com>\n\t<0c1232c4-b41d-4894-a041-bef6ef839b81@ideasonboard.com>","Subject":"Re: [PATCH v3 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 12:24:56 +0000","Message-ID":"<176191349611.3770990.3036548183364784344@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":36578,"web_url":"https://patchwork.libcamera.org/comment/36578/","msgid":"<176191832033.3742839.9633174016165719156@ping.linuxembedded.co.uk>","date":"2025-10-31T13:45:20","subject":"Re: [PATCH v3 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 12:22:35)\n> On Fri, Oct 31, 2025 at 12:59:43PM +0100, Barnabás Pőcze wrote:\n> > 2025. 10. 31. 12:52 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> > > Suggested-by: Barnabás Pőcze \n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > > include/libcamera/base/utils.h | 68 ++++++----------------------------\n> > > src/libcamera/base/utils.cpp | 2 +-\n> > > test/meson.build | 2 +-\n> > > 3 files changed, 14 insertions(+), 58 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> > > index d32bd1cd62e0..555da71f124b 100644\n> > > --- a/include/libcamera/base/utils.h\n> > > +++ b/include/libcamera/base/utils.h\n> > > @@ -78,67 +78,23 @@ 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> * = nullptr>\n> > > + hex(T value, unsigned int width = sizeof(T) * 2)\n> > > + : v(static_cast>(value)),\n> > > + w(width)\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 hex &h);\n> > > +#else\n> > > +template> * = nullptr>\n> > > +void hex(T value, unsigned int width = 0);\n> > > #endif\n> > \n> > I think maybe a question is whether to document the class and its\n> > constructor or inject this function into the documentation.\n> > \n> > But the rest looks good to me.\n> > \n> > Reviewed-by: Barnabás Pőcze \n> \n> This is what we currently document:\n> \n> /**\n> * \\fn hex(T value, unsigned int width)\n> * \\brief Write an hexadecimal value to an output string\n> * \\param value The value\n> * \\param width The width\n> *\n> * Return an object of unspecified type such that, if \\a os is the name of an\n> * output stream of type std::ostream, and T is an integer type, then the\n> * expression\n> *\n> * \\code{.cpp}\n> * os << utils::hex(value)\n> * \\endcode\n> *\n> * will output the \\a value to the stream in hexadecimal form with the base\n> * prefix and the filling character set to '0'. The field width is set to \\a\n> * width if specified to a non-zero value, or to the native width of type T\n> * otherwise. The \\a os stream configuration is not modified.\n> */\n> \n> I prefer hiding the structure as it's really an implementation detail.\n> If we want to avoid cheating, we could do\n> \n> namespace details {\n> \n> struct hex {\n> uint64_t v;\n> unsigned int w;\n> };\n> \n> } /* namespace details */\n> \n> template> * = nullptr>\n> details::_ex hex(T value, unsigned int width = sizeof(T) * 2)\n> {\n> return details::hex{ static_cast>(value)), width };\n> }\n> \n> std::basic_ostream> &\n> operator<<(std::basic_ostream> &stream, const details::hex &h);\n> \n> No need for #ifndef __DOXYGEN__ magic anymore, hex() stays a function\n> compatible with its current documentation, and details::hex is hidden\n> automatically as it's in a details namespace.\n\nPutting\n\ndetails::_ex hex(T value, unsigned int width = sizeof(T) * 2)\n\nbreaks doxygen with things like:\n\nWARNING: Skipping function utils.h::hex(T value, unsigned int width=(sizeof(T) *2)). Error reported from parser was: Expected ')', found '*' (at char 40), (line:1, col:41)\n\n\nSo we may end up with #ifndef __DOXYGEN__ still.\n\n--\nKieran\n\n\n> \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..446c9a05e96d 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 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> \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 9D71FBE080\n\tfor ;\n\tFri, 31 Oct 2025 13:45:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B6466609B3;\n\tFri, 31 Oct 2025 14:45:25 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBE6060947\n\tfor ;\n\tFri, 31 Oct 2025 14:45:23 +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 0BE131691;\n\tFri, 31 Oct 2025 14:43:33 +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=\"nOI7SQ7n\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761918213;\n\tbh=gxkhcs7rAdfTQAIFDzBo5lYR9Q9lYYjKGHKmySUGWIc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=nOI7SQ7ndAJLzj5J71ZTngj2SSo1JInWzCtIwcLj21JFrAC+v1SDsUM4I45clV7tL\n\t4mokepZOmYM1g+chCY89OZ/eApJGh7NVz0IxH+vAjJVtQ0O3eIz4A4Rxk5HR9XhHoH\n\tTv/7IWWY2LBS1uDB1U98f6AjgfhUicKoNWf7syvc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251031122235.GI797@pendragon.ideasonboard.com>","References":"<20251031115216.3776845-1-kieran.bingham@ideasonboard.com>\n\t<20251031115216.3776845-3-kieran.bingham@ideasonboard.com>\n\t<0c1232c4-b41d-4894-a041-bef6ef839b81@ideasonboard.com>\n\t<20251031122235.GI797@pendragon.ideasonboard.com>","Subject":"Re: [PATCH v3 2/2] libcamera: base: utils: Simplify hex adaptor","From":"Kieran Bingham ","Cc":"libcamera devel ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tLaurent Pinchart ","Date":"Fri, 31 Oct 2025 13:45:20 +0000","Message-ID":"<176191832033.3742839.9633174016165719156@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\" "}}]