[v3,2/2] libcamera: base: utils: Simplify hex adaptor

24935 diff mbox series

The libcamera hex string adaptor specifies and casts each type
specifically to map the size of each type.

This needlessly repeats itself for each type and further more has a bug
with signed integer extension which causes values such as 0x80 to be
printed as 0xffffffffffffff80 instead.

Remove the template specialisations for each type, and unify with a
single templated constructor of the struct hex trait.

Suggested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/base/utils.h | 68 +++++++---------------------------
 src/libcamera/base/utils.cpp   |  3 +-
 test/meson.build               |  2 +-
 3 files changed, 17 insertions(+), 56 deletions(-)

On Fri, Oct 31, 2025 at 08:38:54PM +0000, Kieran Bingham wrote:
> The libcamera hex string adaptor specifies and casts each type
> specifically to map the size of each type.
> 
> This needlessly repeats itself for each type and further more has a bug
> with signed integer extension which causes values such as 0x80 to be
> printed as 0xffffffffffffff80 instead.
> 
> Remove the template specialisations for each type, and unify with a
> single templated constructor of the struct hex trait.
> 
> Suggested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  include/libcamera/base/utils.h | 68 +++++++---------------------------
>  src/libcamera/base/utils.cpp   |  3 +-
>  test/meson.build               |  2 +-
>  3 files changed, 17 insertions(+), 56 deletions(-)
> 
> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> index d32bd1cd62e0..0b7407f7724b 100644
> --- a/include/libcamera/base/utils.h
> +++ b/include/libcamera/base/utils.h
> @@ -77,70 +77,30 @@ using time_point = std::chrono::steady_clock::time_point;
>  struct timespec duration_to_timespec(const duration &value);
>  std::string time_point_to_string(const time_point &time);
>  
> -#ifndef __DOXYGEN__
> -struct _hex {
> +namespace details {
> +
> +struct hex {
>  	uint64_t v;
>  	unsigned int w;
>  };
>  
> +template<typename T>
> +constexpr unsigned int hex_width()
> +{
> +	return sizeof(T) * 2;
> +}
> +
>  std::basic_ostream<char, std::char_traits<char>> &
> -operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const _hex &h);
> -#endif
> +operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const hex &h);
>  
> -template<typename T,
> -	 std::enable_if_t<std::is_integral<T>::value> * = nullptr>
> -_hex hex(T value, unsigned int width = 0);
> +} /* namespace details */
>  
> -#ifndef __DOXYGEN__
> -template<>
> -inline _hex hex<int8_t>(int8_t value, unsigned int width)
> +template<typename T, std::enable_if_t<std::is_integral_v<T>> * = nullptr>
> +details::hex hex(T value, unsigned int width = details::hex_width<T>())
>  {
> -	return { static_cast<uint64_t>(value), width ? width : 2 };
> +	return { static_cast<std::make_unsigned_t<T>>(value), width };
>  }
>  
> -template<>
> -inline _hex hex<uint8_t>(uint8_t value, unsigned int width)
> -{
> -	return { static_cast<uint64_t>(value), width ? width : 2 };
> -}
> -
> -template<>
> -inline _hex hex<int16_t>(int16_t value, unsigned int width)
> -{
> -	return { static_cast<uint64_t>(value), width ? width : 4 };
> -}
> -
> -template<>
> -inline _hex hex<uint16_t>(uint16_t value, unsigned int width)
> -{
> -	return { static_cast<uint64_t>(value), width ? width : 4 };
> -}
> -
> -template<>
> -inline _hex hex<int32_t>(int32_t value, unsigned int width)
> -{
> -	return { static_cast<uint64_t>(value), width ? width : 8 };
> -}
> -
> -template<>
> -inline _hex hex<uint32_t>(uint32_t value, unsigned int width)
> -{
> -	return { static_cast<uint64_t>(value), width ? width : 8 };
> -}
> -
> -template<>
> -inline _hex hex<int64_t>(int64_t value, unsigned int width)
> -{
> -	return { static_cast<uint64_t>(value), width ? width : 16 };
> -}
> -
> -template<>
> -inline _hex hex<uint64_t>(uint64_t value, unsigned int width)
> -{
> -	return { static_cast<uint64_t>(value), width ? width : 16 };
> -}
> -#endif
> -
>  size_t strlcpy(char *dst, const char *src, size_t size);
>  
>  #ifndef __DOXYGEN__
> diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp
> index cb9fe0049c83..65d9181c927f 100644
> --- a/src/libcamera/base/utils.cpp
> +++ b/src/libcamera/base/utils.cpp
> @@ -187,7 +187,8 @@ std::string time_point_to_string(const time_point &time)
>  }
>  
>  std::basic_ostream<char, std::char_traits<char>> &
> -operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const _hex &h)
> +details::operator<<(std::basic_ostream<char, std::char_traits<char>> &stream,
> +		    const details::hex &h)
>  {
>  	stream << "0x";
>  
> diff --git a/test/meson.build b/test/meson.build
> index 96c4477f04b2..52f04364e4fc 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -73,7 +73,7 @@ internal_tests = [
>      {'name': 'timer-fail', 'sources': ['timer-fail.cpp'], 'should_fail': true},
>      {'name': 'timer-thread', 'sources': ['timer-thread.cpp']},
>      {'name': 'unique-fd', 'sources': ['unique-fd.cpp']},
> -    {'name': 'utils', 'sources': ['utils.cpp'], 'should_fail': true},
> +    {'name': 'utils', 'sources': ['utils.cpp']},
>      {'name': 'vector', 'sources': ['vector.cpp']},
>      {'name': 'yaml-parser', 'sources': ['yaml-parser.cpp']},
>  ]