| Message ID | 20251030181127.1059501-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Kieran Bingham (2025-10-30 18:11:27) > 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. > And for the credit for the idea ;-) Suggested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/base/utils.h | 66 ++++++---------------------------- > src/libcamera/base/utils.cpp | 2 +- > test/meson.build | 2 +- > 3 files changed, 12 insertions(+), 58 deletions(-) > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h > index d32bd1cd62e0..3b3da7b2a887 100644 > --- a/include/libcamera/base/utils.h > +++ b/include/libcamera/base/utils.h > @@ -78,67 +78,21 @@ 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<T>::value> * = nullptr> > + hex(T value, unsigned int width = 0) > + : v(static_cast<std::make_unsigned_t<T>>(value)), > + w(width ? width : 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 > - > -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 struct hex &h); > +#else > +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..db868be224a2 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 struct 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']}, > ] > -- > 2.50.1 >
Hi 2025. 10. 30. 19:11 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. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/base/utils.h | 66 ++++++---------------------------- > src/libcamera/base/utils.cpp | 2 +- > test/meson.build | 2 +- > 3 files changed, 12 insertions(+), 58 deletions(-) > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h > index d32bd1cd62e0..3b3da7b2a887 100644 > --- a/include/libcamera/base/utils.h > +++ b/include/libcamera/base/utils.h > @@ -78,67 +78,21 @@ 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<T>::value> * = nullptr> This can be `std::is_integral_v<T>`. > + hex(T value, unsigned int width = 0) Better idea... `width = sizeof(T) * 2` > + : v(static_cast<std::make_unsigned_t<T>>(value)), > + w(width ? width : sizeof(T) * 2) ... then this can be just `w(width)` And I think usually the member init list has an extra level of indentation. > + { } > }; > > 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 struct hex &h); Do you need the `struct hex` here? > +#else > +void hex(T value, unsigned int width = 0); I suppose this is to appease doxygen? Does it not need a `template<typename T>`? Regards, Barnabás Pőcze > #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..db868be224a2 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 struct 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 Thu, Oct 30, 2025 at 06:11:27PM +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. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/base/utils.h | 66 ++++++---------------------------- > src/libcamera/base/utils.cpp | 2 +- > test/meson.build | 2 +- > 3 files changed, 12 insertions(+), 58 deletions(-) > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h > index d32bd1cd62e0..3b3da7b2a887 100644 > --- a/include/libcamera/base/utils.h > +++ b/include/libcamera/base/utils.h > @@ -78,67 +78,21 @@ 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<T>::value> * = nullptr> > + hex(T value, unsigned int width = 0) > + : v(static_cast<std::make_unsigned_t<T>>(value)), > + w(width ? width : sizeof(T) * 2) > + { } template<typename T, std::enable_if_t<std::is_integral<T>::value> * = nullptr> hex(T value, unsigned int width = 0) : v(static_cast<std::make_unsigned_t<T>>(value)), w(width ? width : sizeof(T) * 2) { } (our coding style indents the initializers) > }; > > 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 struct hex &h); > +#else > +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..db868be224a2 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 struct hex &h) Any specific reason for adding the "struct" keyword here (and in the .h file) ? > { > 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 09:04:55) > Hi > > > 2025. 10. 30. 19:11 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. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > include/libcamera/base/utils.h | 66 ++++++---------------------------- > > src/libcamera/base/utils.cpp | 2 +- > > test/meson.build | 2 +- > > 3 files changed, 12 insertions(+), 58 deletions(-) > > > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h > > index d32bd1cd62e0..3b3da7b2a887 100644 > > --- a/include/libcamera/base/utils.h > > +++ b/include/libcamera/base/utils.h > > @@ -78,67 +78,21 @@ 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<T>::value> * = nullptr> > > This can be `std::is_integral_v<T>`. > ack > > > + hex(T value, unsigned int width = 0) > > Better idea... `width = sizeof(T) * 2` > > > > + : v(static_cast<std::make_unsigned_t<T>>(value)), > > + w(width ? width : sizeof(T) * 2) > > ... then this can be just `w(width)` > Ah yes that's an nicer solution. > And I think usually the member init list has an extra level of indentation. > ack. > > + { } > > }; > > > > 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 struct hex &h); > > Do you need the `struct hex` here? Reading Laurent's comment I assume you mean the keyword struct. I guess not - I added it because it's now straight to the struct hex, but I guess previously it was struct _hex and worked fine ;D > > +#else > > +void hex(T value, unsigned int width = 0); > > I suppose this is to appease doxygen? Does it not need a `template<typename T>`? Yes, it was to make doxygen be happy. It hasn't complained about template - I'll check again how it builds and adapt / add if needed. > Regards, > Barnabás Pőcze Thanks > > > #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..db868be224a2 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 struct 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 09:07:50) > On Thu, Oct 30, 2025 at 06:11:27PM +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. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > include/libcamera/base/utils.h | 66 ++++++---------------------------- > > src/libcamera/base/utils.cpp | 2 +- > > test/meson.build | 2 +- > > 3 files changed, 12 insertions(+), 58 deletions(-) > > > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h > > index d32bd1cd62e0..3b3da7b2a887 100644 > > --- a/include/libcamera/base/utils.h > > +++ b/include/libcamera/base/utils.h > > @@ -78,67 +78,21 @@ 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<T>::value> * = nullptr> > > + hex(T value, unsigned int width = 0) > > + : v(static_cast<std::make_unsigned_t<T>>(value)), > > + w(width ? width : sizeof(T) * 2) > > + { } > > template<typename T, std::enable_if_t<std::is_integral<T>::value> * = nullptr> > hex(T value, unsigned int width = 0) > : v(static_cast<std::make_unsigned_t<T>>(value)), > w(width ? width : sizeof(T) * 2) > { > } > > (our coding style indents the initializers) I'll update along with Barnabas' suggestions. > > > }; > > > > 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 struct hex &h); > > +#else > > +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..db868be224a2 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 struct hex &h) > > Any specific reason for adding the "struct" keyword here (and in the .h > file) ? > Probably because I thought I had to because I was 'changing the type' ... but given _hex was also a struct ... I think it might work without :D I'll remove and retest for v3. > > { > > 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..3b3da7b2a887 100644 --- a/include/libcamera/base/utils.h +++ b/include/libcamera/base/utils.h @@ -78,67 +78,21 @@ 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<T>::value> * = nullptr> + hex(T value, unsigned int width = 0) + : v(static_cast<std::make_unsigned_t<T>>(value)), + w(width ? width : 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 - -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 struct hex &h); +#else +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..db868be224a2 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 struct 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. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- include/libcamera/base/utils.h | 66 ++++++---------------------------- src/libcamera/base/utils.cpp | 2 +- test/meson.build | 2 +- 3 files changed, 12 insertions(+), 58 deletions(-)