| Message ID | 20251031115216.3776845-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 10. 31. 12:52 keltezéssel, Kieran Bingham írta: > 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 | 2 +- > test/meson.build | 2 +- > 3 files changed, 14 insertions(+), 58 deletions(-) > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h > index d32bd1cd62e0..555da71f124b 100644 > --- a/include/libcamera/base/utils.h > +++ b/include/libcamera/base/utils.h > @@ -78,67 +78,23 @@ struct timespec duration_to_timespec(const duration &value); > std::string time_point_to_string(const time_point &time); > > #ifndef __DOXYGEN__ > -struct _hex { > +struct hex { > uint64_t v; > unsigned int w; > + > + template<typename T, std::enable_if_t<std::is_integral_v<T>> * = nullptr> > + hex(T value, unsigned int width = sizeof(T) * 2) > + : v(static_cast<std::make_unsigned_t<T>>(value)), > + w(width) > + { > + } > }; > > std::basic_ostream<char, std::char_traits<char>> & > -operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const _hex &h); > -#endif > - > -template<typename T, > - std::enable_if_t<std::is_integral<T>::value> * = nullptr> > -_hex hex(T value, unsigned int width = 0); > - > -#ifndef __DOXYGEN__ > -template<> > -inline _hex hex<int8_t>(int8_t value, unsigned int width) > -{ > - return { static_cast<uint64_t>(value), width ? width : 2 }; > -} > - > -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 }; > -} > +operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const hex &h); > +#else > +template<typename T, std::enable_if_t<std::is_integral_v<T>> * = nullptr> > +void hex(T value, unsigned int width = 0); > #endif I think maybe a question is whether to document the class and its constructor or inject this function into the documentation. But the rest looks good to me. Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > size_t strlcpy(char *dst, const char *src, size_t size); > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp > index cb9fe0049c83..446c9a05e96d 100644 > --- a/src/libcamera/base/utils.cpp > +++ b/src/libcamera/base/utils.cpp > @@ -187,7 +187,7 @@ 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) > +operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const 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']}, > ]
On Fri, Oct 31, 2025 at 12:59:43PM +0100, Barnabás Pőcze wrote: > 2025. 10. 31. 12:52 keltezéssel, Kieran Bingham írta: > > 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 | 2 +- > > test/meson.build | 2 +- > > 3 files changed, 14 insertions(+), 58 deletions(-) > > > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h > > index d32bd1cd62e0..555da71f124b 100644 > > --- a/include/libcamera/base/utils.h > > +++ b/include/libcamera/base/utils.h > > @@ -78,67 +78,23 @@ struct timespec duration_to_timespec(const duration &value); > > std::string time_point_to_string(const time_point &time); > > > > #ifndef __DOXYGEN__ > > -struct _hex { > > +struct hex { > > uint64_t v; > > unsigned int w; > > + > > + template<typename T, std::enable_if_t<std::is_integral_v<T>> * = nullptr> > > + hex(T value, unsigned int width = sizeof(T) * 2) > > + : v(static_cast<std::make_unsigned_t<T>>(value)), > > + w(width) > > + { > > + } > > }; > > > > std::basic_ostream<char, std::char_traits<char>> & > > -operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const _hex &h); > > -#endif > > - > > -template<typename T, > > - std::enable_if_t<std::is_integral<T>::value> * = nullptr> > > -_hex hex(T value, unsigned int width = 0); > > - > > -#ifndef __DOXYGEN__ > > -template<> > > -inline _hex hex<int8_t>(int8_t value, unsigned int width) > > -{ > > - return { static_cast<uint64_t>(value), width ? width : 2 }; > > -} > > - > > -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 }; > > -} > > +operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const hex &h); > > +#else > > +template<typename T, std::enable_if_t<std::is_integral_v<T>> * = nullptr> > > +void hex(T value, unsigned int width = 0); > > #endif > > I think maybe a question is whether to document the class and its > constructor or inject this function into the documentation. > > But the rest looks good to me. > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> This is what we currently document: /** * \fn hex(T value, unsigned int width) * \brief Write an hexadecimal value to an output string * \param value The value * \param width The width * * Return an object of unspecified type such that, if \a os is the name of an * output stream of type std::ostream, and T is an integer type, then the * expression * * \code{.cpp} * os << utils::hex(value) * \endcode * * will output the \a value to the stream in hexadecimal form with the base * prefix and the filling character set to '0'. The field width is set to \a * width if specified to a non-zero value, or to the native width of type T * otherwise. The \a os stream configuration is not modified. */ I prefer hiding the structure as it's really an implementation detail. If we want to avoid cheating, we could do namespace details { struct hex { uint64_t v; unsigned int w; }; } /* namespace details */ template<typename T, std::enable_if_t<std::is_integral_v<T>> * = nullptr> details::_ex<T> hex(T value, unsigned int width = sizeof(T) * 2) { return details::hex{ static_cast<std::make_unsigned_t<T>>(value)), width }; } std::basic_ostream<char, std::char_traits<char>> & operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const details::hex &h); No need for #ifndef __DOXYGEN__ magic anymore, hex() stays a function compatible with its current documentation, and details::hex is hidden automatically as it's in a details namespace. > > > > size_t strlcpy(char *dst, const char *src, size_t size); > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp > > index cb9fe0049c83..446c9a05e96d 100644 > > --- a/src/libcamera/base/utils.cpp > > +++ b/src/libcamera/base/utils.cpp > > @@ -187,7 +187,7 @@ 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) > > +operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const 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']}, > > ] >
Quoting Barnabás Pőcze (2025-10-31 11:59:43) > Hi > > 2025. 10. 31. 12:52 keltezéssel, Kieran Bingham írta: > > 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 | 2 +- > > test/meson.build | 2 +- > > 3 files changed, 14 insertions(+), 58 deletions(-) > > > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h > > index d32bd1cd62e0..555da71f124b 100644 > > --- a/include/libcamera/base/utils.h > > +++ b/include/libcamera/base/utils.h > > @@ -78,67 +78,23 @@ struct timespec duration_to_timespec(const duration &value); > > std::string time_point_to_string(const time_point &time); > > > > #ifndef __DOXYGEN__ > > -struct _hex { > > +struct hex { > > uint64_t v; > > unsigned int w; > > + > > + template<typename T, std::enable_if_t<std::is_integral_v<T>> * = nullptr> > > + hex(T value, unsigned int width = sizeof(T) * 2) > > + : v(static_cast<std::make_unsigned_t<T>>(value)), > > + w(width) > > + { > > + } > > }; > > > > std::basic_ostream<char, std::char_traits<char>> & > > -operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const _hex &h); > > -#endif > > - > > -template<typename T, > > - std::enable_if_t<std::is_integral<T>::value> * = nullptr> > > -_hex hex(T value, unsigned int width = 0); > > - > > -#ifndef __DOXYGEN__ > > -template<> > > -inline _hex hex<int8_t>(int8_t value, unsigned int width) > > -{ > > - return { static_cast<uint64_t>(value), width ? width : 2 }; > > -} > > - > > -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 }; > > -} > > +operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const hex &h); > > +#else > > +template<typename T, std::enable_if_t<std::is_integral_v<T>> * = nullptr> > > +void hex(T value, unsigned int width = 0); > > #endif > > I think maybe a question is whether to document the class and its > constructor or inject this function into the documentation. It would require us documenting w and v ... and I think that's internal implementation detail - I was happy with how the documentation was phrased already - so this just keeps that alignment. It's the equivalent of these lines which were removed, except because now it's a constructor it's void ... or just no return at all. > > -template<typename T, > > - std::enable_if_t<std::is_integral<T>::value> * = nullptr> > > -_hex hex(T value, unsigned int width = 0); -- Kieran > > But the rest looks good to me. > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > > > > size_t strlcpy(char *dst, const char *src, size_t size); > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp > > index cb9fe0049c83..446c9a05e96d 100644 > > --- a/src/libcamera/base/utils.cpp > > +++ b/src/libcamera/base/utils.cpp > > @@ -187,7 +187,7 @@ 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) > > +operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const 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']}, > > ] >
Quoting Laurent Pinchart (2025-10-31 12:22:35) > On Fri, Oct 31, 2025 at 12:59:43PM +0100, Barnabás Pőcze wrote: > > 2025. 10. 31. 12:52 keltezéssel, Kieran Bingham írta: > > > 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 | 2 +- > > > test/meson.build | 2 +- > > > 3 files changed, 14 insertions(+), 58 deletions(-) > > > > > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h > > > index d32bd1cd62e0..555da71f124b 100644 > > > --- a/include/libcamera/base/utils.h > > > +++ b/include/libcamera/base/utils.h > > > @@ -78,67 +78,23 @@ struct timespec duration_to_timespec(const duration &value); > > > std::string time_point_to_string(const time_point &time); > > > > > > #ifndef __DOXYGEN__ > > > -struct _hex { > > > +struct hex { > > > uint64_t v; > > > unsigned int w; > > > + > > > + template<typename T, std::enable_if_t<std::is_integral_v<T>> * = nullptr> > > > + hex(T value, unsigned int width = sizeof(T) * 2) > > > + : v(static_cast<std::make_unsigned_t<T>>(value)), > > > + w(width) > > > + { > > > + } > > > }; > > > > > > std::basic_ostream<char, std::char_traits<char>> & > > > -operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const _hex &h); > > > -#endif > > > - > > > -template<typename T, > > > - std::enable_if_t<std::is_integral<T>::value> * = nullptr> > > > -_hex hex(T value, unsigned int width = 0); > > > - > > > -#ifndef __DOXYGEN__ > > > -template<> > > > -inline _hex hex<int8_t>(int8_t value, unsigned int width) > > > -{ > > > - return { static_cast<uint64_t>(value), width ? width : 2 }; > > > -} > > > - > > > -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 }; > > > -} > > > +operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const hex &h); > > > +#else > > > +template<typename T, std::enable_if_t<std::is_integral_v<T>> * = nullptr> > > > +void hex(T value, unsigned int width = 0); > > > #endif > > > > I think maybe a question is whether to document the class and its > > constructor or inject this function into the documentation. > > > > But the rest looks good to me. > > > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > This is what we currently document: > > /** > * \fn hex(T value, unsigned int width) > * \brief Write an hexadecimal value to an output string > * \param value The value > * \param width The width > * > * Return an object of unspecified type such that, if \a os is the name of an > * output stream of type std::ostream, and T is an integer type, then the > * expression > * > * \code{.cpp} > * os << utils::hex(value) > * \endcode > * > * will output the \a value to the stream in hexadecimal form with the base > * prefix and the filling character set to '0'. The field width is set to \a > * width if specified to a non-zero value, or to the native width of type T > * otherwise. The \a os stream configuration is not modified. > */ > > I prefer hiding the structure as it's really an implementation detail. > If we want to avoid cheating, we could do > > namespace details { > > struct hex { > uint64_t v; > unsigned int w; > }; > > } /* namespace details */ > > template<typename T, std::enable_if_t<std::is_integral_v<T>> * = nullptr> > details::_ex<T> hex(T value, unsigned int width = sizeof(T) * 2) > { > return details::hex{ static_cast<std::make_unsigned_t<T>>(value)), width }; > } > > std::basic_ostream<char, std::char_traits<char>> & > operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const details::hex &h); > > No need for #ifndef __DOXYGEN__ magic anymore, hex() stays a function > compatible with its current documentation, and details::hex is hidden > automatically as it's in a details namespace. Putting details::_ex<T> hex(T value, unsigned int width = sizeof(T) * 2) breaks doxygen with things like: WARNING: 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) So we may end up with #ifndef __DOXYGEN__ still. -- Kieran > > > > > > > size_t strlcpy(char *dst, const char *src, size_t size); > > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp > > > index cb9fe0049c83..446c9a05e96d 100644 > > > --- a/src/libcamera/base/utils.cpp > > > +++ b/src/libcamera/base/utils.cpp > > > @@ -187,7 +187,7 @@ 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) > > > +operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const 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']}, > > > ] > > > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h index d32bd1cd62e0..555da71f124b 100644 --- a/include/libcamera/base/utils.h +++ b/include/libcamera/base/utils.h @@ -78,67 +78,23 @@ struct timespec duration_to_timespec(const duration &value); std::string time_point_to_string(const time_point &time); #ifndef __DOXYGEN__ -struct _hex { +struct hex { uint64_t v; unsigned int w; + + template<typename T, std::enable_if_t<std::is_integral_v<T>> * = nullptr> + hex(T value, unsigned int width = sizeof(T) * 2) + : v(static_cast<std::make_unsigned_t<T>>(value)), + w(width) + { + } }; std::basic_ostream<char, std::char_traits<char>> & -operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const _hex &h); -#endif - -template<typename T, - std::enable_if_t<std::is_integral<T>::value> * = nullptr> -_hex hex(T value, unsigned int width = 0); - -#ifndef __DOXYGEN__ -template<> -inline _hex hex<int8_t>(int8_t value, unsigned int width) -{ - return { static_cast<uint64_t>(value), width ? width : 2 }; -} - -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 }; -} +operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const hex &h); +#else +template<typename T, std::enable_if_t<std::is_integral_v<T>> * = nullptr> +void hex(T value, unsigned int width = 0); #endif size_t strlcpy(char *dst, const char *src, size_t size); diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp index cb9fe0049c83..446c9a05e96d 100644 --- a/src/libcamera/base/utils.cpp +++ b/src/libcamera/base/utils.cpp @@ -187,7 +187,7 @@ 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) +operator<<(std::basic_ostream<char, std::char_traits<char>> &stream, const 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']}, ]
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 | 2 +- test/meson.build | 2 +- 3 files changed, 14 insertions(+), 58 deletions(-)