Message ID | 20241112185637.10232-1-laurent.pinchart@ideasonboard.com |
---|---|
State | Accepted |
Commit | 5c71df927ddaaa01204bff1e647c9d2bf653d95f |
Headers | show |
Series |
|
Related | show |
Hi Laurent On Tue, Nov 12, 2024 at 08:56:37PM +0200, Laurent Pinchart wrote: > std::from_chars(), introduced in C++17, is a fast, locale-independent > string-to-arithmetic conversion function. The C++ standard library > provides overloads for all integer types, making it a prime candidate to > replace the manual handling of integer sizes in the YamlParser string to > integer conversion. > > Compared to std::strtol(), std::from_chars() doesn't recognize the '0x' only if the base is 16, or do I read https://en.cppreference.com/w/cpp/utility/from_chars wrong > prefix or '+' prefix, and doesn't ignore leading white space. As the > YamlParser doesn't require those features, std::from_chars() can be used > safely, reducing the amount of code. Ok, I presume we won't have "0x.." entries in config files ? > > C++17 also requires the standard C++ library to provide overloads for > floating-point types, but libc++ does not implement those. The float and Do you know why ? > bool implementations of YamlParser::Getter::get() are therefore kept > as-is. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/internal/yaml_parser.h | 2 +- > src/libcamera/yaml_parser.cpp | 168 ++++------------------- > 2 files changed, 29 insertions(+), 141 deletions(-) > > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h > index de452844fe0a..8c7916565946 100644 > --- a/include/libcamera/internal/yaml_parser.h > +++ b/include/libcamera/internal/yaml_parser.h > @@ -224,7 +224,7 @@ private: > Empty, > }; > > - template<typename T> > + template<typename T, typename Enable = void> > struct Getter { > std::optional<T> get(const YamlObject &obj) const; > }; > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp > index 7c0b341a32e7..db256ec5b04d 100644 > --- a/src/libcamera/yaml_parser.cpp > +++ b/src/libcamera/yaml_parser.cpp > @@ -7,6 +7,7 @@ > > #include "libcamera/internal/yaml_parser.h" > > +#include <charconv> > #include <cstdlib> > #include <errno.h> > #include <functional> > @@ -146,151 +147,38 @@ YamlObject::Getter<bool>::get(const YamlObject &obj) const > return std::nullopt; > } > > -namespace { > - > -bool parseSignedInteger(const std::string &str, long min, long max, out-of-range conversion is now handled by std::from_chars(), nice > - long *result) > +template<typename T> > +struct YamlObject::Getter<T, std::enable_if_t< > + std::is_same_v<int8_t, T> || > + std::is_same_v<uint8_t, T> || > + std::is_same_v<int16_t, T> || > + std::is_same_v<uint16_t, T> || > + std::is_same_v<int32_t, T> || > + std::is_same_v<uint32_t, T>>> > { > - if (str == "") > - return false; > + std::optional<T> get(const YamlObject &obj) const > + { > + if (obj.type_ != Type::Value) > + return std::nullopt; > > - char *end; > + const std::string &str = obj.value_; > + T value; > > - errno = 0; > - long value = std::strtol(str.c_str(), &end, 10); > + auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(), > + value); > + if (ptr != str.data() + str.size() || ec != std::errc()) > + return std::nullopt; > > - if ('\0' != *end || errno == ERANGE || value < min || value > max) > - return false; > + return value; > + } > +}; > > - *result = value; > - return true; > -} > - > -bool parseUnsignedInteger(const std::string &str, unsigned long max, > - unsigned long *result) > -{ > - if (str == "") > - return false; > - > - /* > - * strtoul() accepts strings representing a negative number, in which > - * case it negates the converted value. We don't want to silently accept > - * negative values and return a large positive number, so check for a > - * minus sign (after optional whitespace) and return an error. > - */ > - std::size_t found = str.find_first_not_of(" \t"); > - if (found != std::string::npos && str[found] == '-') > - return false; > - > - char *end; > - > - errno = 0; > - unsigned long value = std::strtoul(str.c_str(), &end, 10); > - > - if ('\0' != *end || errno == ERANGE || value > max) > - return false; > - > - *result = value; > - return true; > -} > - > -} /* namespace */ > - > -template<> > -std::optional<int8_t> > -YamlObject::Getter<int8_t>::get(const YamlObject &obj) const > -{ > - if (obj.type_ != Type::Value) > - return std::nullopt; > - > - long value; > - > - if (!parseSignedInteger(obj.value_, std::numeric_limits<int8_t>::min(), > - std::numeric_limits<int8_t>::max(), &value)) > - return std::nullopt; > - > - return value; > -} > - > -template<> > -std::optional<uint8_t> > -YamlObject::Getter<uint8_t>::get(const YamlObject &obj) const > -{ > - if (obj.type_ != Type::Value) > - return std::nullopt; > - > - unsigned long value; > - > - if (!parseUnsignedInteger(obj.value_, std::numeric_limits<uint8_t>::max(), > - &value)) > - return std::nullopt; > - > - return value; > -} > - > -template<> > -std::optional<int16_t> > -YamlObject::Getter<int16_t>::get(const YamlObject &obj) const > -{ > - if (obj.type_ != Type::Value) > - return std::nullopt; > - > - long value; > - > - if (!parseSignedInteger(obj.value_, std::numeric_limits<int16_t>::min(), > - std::numeric_limits<int16_t>::max(), &value)) > - return std::nullopt; > - > - return value; > -} > - > -template<> > -std::optional<uint16_t> > -YamlObject::Getter<uint16_t>::get(const YamlObject &obj) const > -{ > - if (obj.type_ != Type::Value) > - return std::nullopt; > - > - unsigned long value; > - > - if (!parseUnsignedInteger(obj.value_, std::numeric_limits<uint16_t>::max(), > - &value)) > - return std::nullopt; > - > - return value; > -} > - > -template<> > -std::optional<int32_t> > -YamlObject::Getter<int32_t>::get(const YamlObject &obj) const > -{ > - if (obj.type_ != Type::Value) > - return std::nullopt; > - > - long value; > - > - if (!parseSignedInteger(obj.value_, std::numeric_limits<int32_t>::min(), > - std::numeric_limits<int32_t>::max(), &value)) > - return std::nullopt; > - > - return value; > -} > - > -template<> > -std::optional<uint32_t> > -YamlObject::Getter<uint32_t>::get(const YamlObject &obj) const > -{ > - if (obj.type_ != Type::Value) > - return std::nullopt; > - > - unsigned long value; > - > - if (!parseUnsignedInteger(obj.value_, std::numeric_limits<uint32_t>::max(), > - &value)) > - return std::nullopt; > - > - return value; > -} > +template struct YamlObject::Getter<int8_t>; > +template struct YamlObject::Getter<uint8_t>; > +template struct YamlObject::Getter<int16_t>; > +template struct YamlObject::Getter<uint16_t>; > +template struct YamlObject::Getter<int32_t>; > +template struct YamlObject::Getter<uint32_t>; > > template<> > std::optional<float> > > base-commit: dcb90f13cf79d909d04dacf2cfbd256a4744347f Looks good to me Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > -- > Regards, > > Laurent Pinchart >
On Wed, Nov 13, 2024 at 12:39:01PM +0100, Jacopo Mondi wrote: > Hi Laurent > > On Tue, Nov 12, 2024 at 08:56:37PM +0200, Laurent Pinchart wrote: > > std::from_chars(), introduced in C++17, is a fast, locale-independent > > string-to-arithmetic conversion function. The C++ standard library > > provides overloads for all integer types, making it a prime candidate to > > replace the manual handling of integer sizes in the YamlParser string to > > integer conversion. > > > > Compared to std::strtol(), std::from_chars() doesn't recognize the '0x' > > only if the base is 16, or do I read > https://en.cppreference.com/w/cpp/utility/from_chars > wrong You're correct. If the base is 10, there should be no '0x' prefix anyway. > > prefix or '+' prefix, and doesn't ignore leading white space. As the > > YamlParser doesn't require those features, std::from_chars() can be used > > safely, reducing the amount of code. > > Ok, I presume we won't have "0x.." entries in config files ? I wouldn't assume so, as we explicitly specify base 10 in the strtol() call. > > C++17 also requires the standard C++ library to provide overloads for > > floating-point types, but libc++ does not implement those. The float and > > Do you know why ? I believe it's more complicated than it may seem. The C++ standard requires that the roundtrip std::from_chars(std::to_chars(value)) produces the exact same value, and apparently it's not trivial. > > bool implementations of YamlParser::Getter::get() are therefore kept > > as-is. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/internal/yaml_parser.h | 2 +- > > src/libcamera/yaml_parser.cpp | 168 ++++------------------- > > 2 files changed, 29 insertions(+), 141 deletions(-) > > > > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h > > index de452844fe0a..8c7916565946 100644 > > --- a/include/libcamera/internal/yaml_parser.h > > +++ b/include/libcamera/internal/yaml_parser.h > > @@ -224,7 +224,7 @@ private: > > Empty, > > }; > > > > - template<typename T> > > + template<typename T, typename Enable = void> > > struct Getter { > > std::optional<T> get(const YamlObject &obj) const; > > }; > > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp > > index 7c0b341a32e7..db256ec5b04d 100644 > > --- a/src/libcamera/yaml_parser.cpp > > +++ b/src/libcamera/yaml_parser.cpp > > @@ -7,6 +7,7 @@ > > > > #include "libcamera/internal/yaml_parser.h" > > > > +#include <charconv> > > #include <cstdlib> > > #include <errno.h> > > #include <functional> > > @@ -146,151 +147,38 @@ YamlObject::Getter<bool>::get(const YamlObject &obj) const > > return std::nullopt; > > } > > > > -namespace { > > - > > -bool parseSignedInteger(const std::string &str, long min, long max, > > out-of-range conversion is now handled by std::from_chars(), nice > > > - long *result) > > +template<typename T> > > +struct YamlObject::Getter<T, std::enable_if_t< > > + std::is_same_v<int8_t, T> || > > + std::is_same_v<uint8_t, T> || > > + std::is_same_v<int16_t, T> || > > + std::is_same_v<uint16_t, T> || > > + std::is_same_v<int32_t, T> || > > + std::is_same_v<uint32_t, T>>> > > { > > - if (str == "") > > - return false; > > + std::optional<T> get(const YamlObject &obj) const > > + { > > + if (obj.type_ != Type::Value) > > + return std::nullopt; > > > > - char *end; > > + const std::string &str = obj.value_; > > + T value; > > > > - errno = 0; > > - long value = std::strtol(str.c_str(), &end, 10); > > + auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(), > > + value); > > + if (ptr != str.data() + str.size() || ec != std::errc()) > > + return std::nullopt; > > > > - if ('\0' != *end || errno == ERANGE || value < min || value > max) > > - return false; > > + return value; > > + } > > +}; > > > > - *result = value; > > - return true; > > -} > > - > > -bool parseUnsignedInteger(const std::string &str, unsigned long max, > > - unsigned long *result) > > -{ > > - if (str == "") > > - return false; > > - > > - /* > > - * strtoul() accepts strings representing a negative number, in which > > - * case it negates the converted value. We don't want to silently accept > > - * negative values and return a large positive number, so check for a > > - * minus sign (after optional whitespace) and return an error. > > - */ > > - std::size_t found = str.find_first_not_of(" \t"); > > - if (found != std::string::npos && str[found] == '-') > > - return false; > > - > > - char *end; > > - > > - errno = 0; > > - unsigned long value = std::strtoul(str.c_str(), &end, 10); > > - > > - if ('\0' != *end || errno == ERANGE || value > max) > > - return false; > > - > > - *result = value; > > - return true; > > -} > > - > > -} /* namespace */ > > - > > -template<> > > -std::optional<int8_t> > > -YamlObject::Getter<int8_t>::get(const YamlObject &obj) const > > -{ > > - if (obj.type_ != Type::Value) > > - return std::nullopt; > > - > > - long value; > > - > > - if (!parseSignedInteger(obj.value_, std::numeric_limits<int8_t>::min(), > > - std::numeric_limits<int8_t>::max(), &value)) > > - return std::nullopt; > > - > > - return value; > > -} > > - > > -template<> > > -std::optional<uint8_t> > > -YamlObject::Getter<uint8_t>::get(const YamlObject &obj) const > > -{ > > - if (obj.type_ != Type::Value) > > - return std::nullopt; > > - > > - unsigned long value; > > - > > - if (!parseUnsignedInteger(obj.value_, std::numeric_limits<uint8_t>::max(), > > - &value)) > > - return std::nullopt; > > - > > - return value; > > -} > > - > > -template<> > > -std::optional<int16_t> > > -YamlObject::Getter<int16_t>::get(const YamlObject &obj) const > > -{ > > - if (obj.type_ != Type::Value) > > - return std::nullopt; > > - > > - long value; > > - > > - if (!parseSignedInteger(obj.value_, std::numeric_limits<int16_t>::min(), > > - std::numeric_limits<int16_t>::max(), &value)) > > - return std::nullopt; > > - > > - return value; > > -} > > - > > -template<> > > -std::optional<uint16_t> > > -YamlObject::Getter<uint16_t>::get(const YamlObject &obj) const > > -{ > > - if (obj.type_ != Type::Value) > > - return std::nullopt; > > - > > - unsigned long value; > > - > > - if (!parseUnsignedInteger(obj.value_, std::numeric_limits<uint16_t>::max(), > > - &value)) > > - return std::nullopt; > > - > > - return value; > > -} > > - > > -template<> > > -std::optional<int32_t> > > -YamlObject::Getter<int32_t>::get(const YamlObject &obj) const > > -{ > > - if (obj.type_ != Type::Value) > > - return std::nullopt; > > - > > - long value; > > - > > - if (!parseSignedInteger(obj.value_, std::numeric_limits<int32_t>::min(), > > - std::numeric_limits<int32_t>::max(), &value)) > > - return std::nullopt; > > - > > - return value; > > -} > > - > > -template<> > > -std::optional<uint32_t> > > -YamlObject::Getter<uint32_t>::get(const YamlObject &obj) const > > -{ > > - if (obj.type_ != Type::Value) > > - return std::nullopt; > > - > > - unsigned long value; > > - > > - if (!parseUnsignedInteger(obj.value_, std::numeric_limits<uint32_t>::max(), > > - &value)) > > - return std::nullopt; > > - > > - return value; > > -} > > +template struct YamlObject::Getter<int8_t>; > > +template struct YamlObject::Getter<uint8_t>; > > +template struct YamlObject::Getter<int16_t>; > > +template struct YamlObject::Getter<uint16_t>; > > +template struct YamlObject::Getter<int32_t>; > > +template struct YamlObject::Getter<uint32_t>; > > > > template<> > > std::optional<float> > > > > base-commit: dcb90f13cf79d909d04dacf2cfbd256a4744347f > > Looks good to me > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Quoting Laurent Pinchart (2024-11-13 11:43:12) > On Wed, Nov 13, 2024 at 12:39:01PM +0100, Jacopo Mondi wrote: > > Hi Laurent > > > > On Tue, Nov 12, 2024 at 08:56:37PM +0200, Laurent Pinchart wrote: > > > std::from_chars(), introduced in C++17, is a fast, locale-independent > > > string-to-arithmetic conversion function. The C++ standard library > > > provides overloads for all integer types, making it a prime candidate to > > > replace the manual handling of integer sizes in the YamlParser string to > > > integer conversion. > > > > > > Compared to std::strtol(), std::from_chars() doesn't recognize the '0x' > > > > only if the base is 16, or do I read > > https://en.cppreference.com/w/cpp/utility/from_chars > > wrong > > You're correct. If the base is 10, there should be no '0x' prefix > anyway. > > > > prefix or '+' prefix, and doesn't ignore leading white space. As the > > > YamlParser doesn't require those features, std::from_chars() can be used > > > safely, reducing the amount of code. > > > > Ok, I presume we won't have "0x.." entries in config files ? > > I wouldn't assume so, as we explicitly specify base 10 in the strtol() > call. > > > > C++17 also requires the standard C++ library to provide overloads for > > > floating-point types, but libc++ does not implement those. The float and > > > > Do you know why ? > > I believe it's more complicated than it may seem. The C++ standard > requires that the roundtrip std::from_chars(std::to_chars(value)) > produces the exact same value, and apparently it's not trivial. > > > > bool implementations of YamlParser::Getter::get() are therefore kept > > > as-is. My first thought reading this was ... does this mean we have to have 'two implementations' of something, but the result below does seem to show a worthwhile code reduction so : Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > include/libcamera/internal/yaml_parser.h | 2 +- > > > src/libcamera/yaml_parser.cpp | 168 ++++------------------- > > > 2 files changed, 29 insertions(+), 141 deletions(-) > > > > > > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h > > > index de452844fe0a..8c7916565946 100644 > > > --- a/include/libcamera/internal/yaml_parser.h > > > +++ b/include/libcamera/internal/yaml_parser.h > > > @@ -224,7 +224,7 @@ private: > > > Empty, > > > }; > > > > > > - template<typename T> > > > + template<typename T, typename Enable = void> > > > struct Getter { > > > std::optional<T> get(const YamlObject &obj) const; > > > }; > > > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp > > > index 7c0b341a32e7..db256ec5b04d 100644 > > > --- a/src/libcamera/yaml_parser.cpp > > > +++ b/src/libcamera/yaml_parser.cpp > > > @@ -7,6 +7,7 @@ > > > > > > #include "libcamera/internal/yaml_parser.h" > > > > > > +#include <charconv> > > > #include <cstdlib> > > > #include <errno.h> > > > #include <functional> > > > @@ -146,151 +147,38 @@ YamlObject::Getter<bool>::get(const YamlObject &obj) const > > > return std::nullopt; > > > } > > > > > > -namespace { > > > - > > > -bool parseSignedInteger(const std::string &str, long min, long max, > > > > out-of-range conversion is now handled by std::from_chars(), nice > > > > > - long *result) > > > +template<typename T> > > > +struct YamlObject::Getter<T, std::enable_if_t< > > > + std::is_same_v<int8_t, T> || > > > + std::is_same_v<uint8_t, T> || > > > + std::is_same_v<int16_t, T> || > > > + std::is_same_v<uint16_t, T> || > > > + std::is_same_v<int32_t, T> || > > > + std::is_same_v<uint32_t, T>>> > > > { > > > - if (str == "") > > > - return false; > > > + std::optional<T> get(const YamlObject &obj) const > > > + { > > > + if (obj.type_ != Type::Value) > > > + return std::nullopt; > > > > > > - char *end; > > > + const std::string &str = obj.value_; > > > + T value; > > > > > > - errno = 0; > > > - long value = std::strtol(str.c_str(), &end, 10); > > > + auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(), > > > + value); > > > + if (ptr != str.data() + str.size() || ec != std::errc()) > > > + return std::nullopt; > > > > > > - if ('\0' != *end || errno == ERANGE || value < min || value > max) > > > - return false; > > > + return value; > > > + } > > > +}; > > > > > > - *result = value; > > > - return true; > > > -} > > > - > > > -bool parseUnsignedInteger(const std::string &str, unsigned long max, > > > - unsigned long *result) > > > -{ > > > - if (str == "") > > > - return false; > > > - > > > - /* > > > - * strtoul() accepts strings representing a negative number, in which > > > - * case it negates the converted value. We don't want to silently accept > > > - * negative values and return a large positive number, so check for a > > > - * minus sign (after optional whitespace) and return an error. > > > - */ > > > - std::size_t found = str.find_first_not_of(" \t"); > > > - if (found != std::string::npos && str[found] == '-') > > > - return false; > > > - > > > - char *end; > > > - > > > - errno = 0; > > > - unsigned long value = std::strtoul(str.c_str(), &end, 10); > > > - > > > - if ('\0' != *end || errno == ERANGE || value > max) > > > - return false; > > > - > > > - *result = value; > > > - return true; > > > -} > > > - > > > -} /* namespace */ > > > - > > > -template<> > > > -std::optional<int8_t> > > > -YamlObject::Getter<int8_t>::get(const YamlObject &obj) const > > > -{ > > > - if (obj.type_ != Type::Value) > > > - return std::nullopt; > > > - > > > - long value; > > > - > > > - if (!parseSignedInteger(obj.value_, std::numeric_limits<int8_t>::min(), > > > - std::numeric_limits<int8_t>::max(), &value)) > > > - return std::nullopt; > > > - > > > - return value; > > > -} > > > - > > > -template<> > > > -std::optional<uint8_t> > > > -YamlObject::Getter<uint8_t>::get(const YamlObject &obj) const > > > -{ > > > - if (obj.type_ != Type::Value) > > > - return std::nullopt; > > > - > > > - unsigned long value; > > > - > > > - if (!parseUnsignedInteger(obj.value_, std::numeric_limits<uint8_t>::max(), > > > - &value)) > > > - return std::nullopt; > > > - > > > - return value; > > > -} > > > - > > > -template<> > > > -std::optional<int16_t> > > > -YamlObject::Getter<int16_t>::get(const YamlObject &obj) const > > > -{ > > > - if (obj.type_ != Type::Value) > > > - return std::nullopt; > > > - > > > - long value; > > > - > > > - if (!parseSignedInteger(obj.value_, std::numeric_limits<int16_t>::min(), > > > - std::numeric_limits<int16_t>::max(), &value)) > > > - return std::nullopt; > > > - > > > - return value; > > > -} > > > - > > > -template<> > > > -std::optional<uint16_t> > > > -YamlObject::Getter<uint16_t>::get(const YamlObject &obj) const > > > -{ > > > - if (obj.type_ != Type::Value) > > > - return std::nullopt; > > > - > > > - unsigned long value; > > > - > > > - if (!parseUnsignedInteger(obj.value_, std::numeric_limits<uint16_t>::max(), > > > - &value)) > > > - return std::nullopt; > > > - > > > - return value; > > > -} > > > - > > > -template<> > > > -std::optional<int32_t> > > > -YamlObject::Getter<int32_t>::get(const YamlObject &obj) const > > > -{ > > > - if (obj.type_ != Type::Value) > > > - return std::nullopt; > > > - > > > - long value; > > > - > > > - if (!parseSignedInteger(obj.value_, std::numeric_limits<int32_t>::min(), > > > - std::numeric_limits<int32_t>::max(), &value)) > > > - return std::nullopt; > > > - > > > - return value; > > > -} > > > - > > > -template<> > > > -std::optional<uint32_t> > > > -YamlObject::Getter<uint32_t>::get(const YamlObject &obj) const > > > -{ > > > - if (obj.type_ != Type::Value) > > > - return std::nullopt; > > > - > > > - unsigned long value; > > > - > > > - if (!parseUnsignedInteger(obj.value_, std::numeric_limits<uint32_t>::max(), > > > - &value)) > > > - return std::nullopt; > > > - > > > - return value; > > > -} > > > +template struct YamlObject::Getter<int8_t>; > > > +template struct YamlObject::Getter<uint8_t>; > > > +template struct YamlObject::Getter<int16_t>; > > > +template struct YamlObject::Getter<uint16_t>; > > > +template struct YamlObject::Getter<int32_t>; > > > +template struct YamlObject::Getter<uint32_t>; > > > > > > template<> > > > std::optional<float> > > > > > > base-commit: dcb90f13cf79d909d04dacf2cfbd256a4744347f > > > > Looks good to me > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h index de452844fe0a..8c7916565946 100644 --- a/include/libcamera/internal/yaml_parser.h +++ b/include/libcamera/internal/yaml_parser.h @@ -224,7 +224,7 @@ private: Empty, }; - template<typename T> + template<typename T, typename Enable = void> struct Getter { std::optional<T> get(const YamlObject &obj) const; }; diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp index 7c0b341a32e7..db256ec5b04d 100644 --- a/src/libcamera/yaml_parser.cpp +++ b/src/libcamera/yaml_parser.cpp @@ -7,6 +7,7 @@ #include "libcamera/internal/yaml_parser.h" +#include <charconv> #include <cstdlib> #include <errno.h> #include <functional> @@ -146,151 +147,38 @@ YamlObject::Getter<bool>::get(const YamlObject &obj) const return std::nullopt; } -namespace { - -bool parseSignedInteger(const std::string &str, long min, long max, - long *result) +template<typename T> +struct YamlObject::Getter<T, std::enable_if_t< + std::is_same_v<int8_t, T> || + std::is_same_v<uint8_t, T> || + std::is_same_v<int16_t, T> || + std::is_same_v<uint16_t, T> || + std::is_same_v<int32_t, T> || + std::is_same_v<uint32_t, T>>> { - if (str == "") - return false; + std::optional<T> get(const YamlObject &obj) const + { + if (obj.type_ != Type::Value) + return std::nullopt; - char *end; + const std::string &str = obj.value_; + T value; - errno = 0; - long value = std::strtol(str.c_str(), &end, 10); + auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(), + value); + if (ptr != str.data() + str.size() || ec != std::errc()) + return std::nullopt; - if ('\0' != *end || errno == ERANGE || value < min || value > max) - return false; + return value; + } +}; - *result = value; - return true; -} - -bool parseUnsignedInteger(const std::string &str, unsigned long max, - unsigned long *result) -{ - if (str == "") - return false; - - /* - * strtoul() accepts strings representing a negative number, in which - * case it negates the converted value. We don't want to silently accept - * negative values and return a large positive number, so check for a - * minus sign (after optional whitespace) and return an error. - */ - std::size_t found = str.find_first_not_of(" \t"); - if (found != std::string::npos && str[found] == '-') - return false; - - char *end; - - errno = 0; - unsigned long value = std::strtoul(str.c_str(), &end, 10); - - if ('\0' != *end || errno == ERANGE || value > max) - return false; - - *result = value; - return true; -} - -} /* namespace */ - -template<> -std::optional<int8_t> -YamlObject::Getter<int8_t>::get(const YamlObject &obj) const -{ - if (obj.type_ != Type::Value) - return std::nullopt; - - long value; - - if (!parseSignedInteger(obj.value_, std::numeric_limits<int8_t>::min(), - std::numeric_limits<int8_t>::max(), &value)) - return std::nullopt; - - return value; -} - -template<> -std::optional<uint8_t> -YamlObject::Getter<uint8_t>::get(const YamlObject &obj) const -{ - if (obj.type_ != Type::Value) - return std::nullopt; - - unsigned long value; - - if (!parseUnsignedInteger(obj.value_, std::numeric_limits<uint8_t>::max(), - &value)) - return std::nullopt; - - return value; -} - -template<> -std::optional<int16_t> -YamlObject::Getter<int16_t>::get(const YamlObject &obj) const -{ - if (obj.type_ != Type::Value) - return std::nullopt; - - long value; - - if (!parseSignedInteger(obj.value_, std::numeric_limits<int16_t>::min(), - std::numeric_limits<int16_t>::max(), &value)) - return std::nullopt; - - return value; -} - -template<> -std::optional<uint16_t> -YamlObject::Getter<uint16_t>::get(const YamlObject &obj) const -{ - if (obj.type_ != Type::Value) - return std::nullopt; - - unsigned long value; - - if (!parseUnsignedInteger(obj.value_, std::numeric_limits<uint16_t>::max(), - &value)) - return std::nullopt; - - return value; -} - -template<> -std::optional<int32_t> -YamlObject::Getter<int32_t>::get(const YamlObject &obj) const -{ - if (obj.type_ != Type::Value) - return std::nullopt; - - long value; - - if (!parseSignedInteger(obj.value_, std::numeric_limits<int32_t>::min(), - std::numeric_limits<int32_t>::max(), &value)) - return std::nullopt; - - return value; -} - -template<> -std::optional<uint32_t> -YamlObject::Getter<uint32_t>::get(const YamlObject &obj) const -{ - if (obj.type_ != Type::Value) - return std::nullopt; - - unsigned long value; - - if (!parseUnsignedInteger(obj.value_, std::numeric_limits<uint32_t>::max(), - &value)) - return std::nullopt; - - return value; -} +template struct YamlObject::Getter<int8_t>; +template struct YamlObject::Getter<uint8_t>; +template struct YamlObject::Getter<int16_t>; +template struct YamlObject::Getter<uint16_t>; +template struct YamlObject::Getter<int32_t>; +template struct YamlObject::Getter<uint32_t>; template<> std::optional<float>
std::from_chars(), introduced in C++17, is a fast, locale-independent string-to-arithmetic conversion function. The C++ standard library provides overloads for all integer types, making it a prime candidate to replace the manual handling of integer sizes in the YamlParser string to integer conversion. Compared to std::strtol(), std::from_chars() doesn't recognize the '0x' prefix or '+' prefix, and doesn't ignore leading white space. As the YamlParser doesn't require those features, std::from_chars() can be used safely, reducing the amount of code. C++17 also requires the standard C++ library to provide overloads for floating-point types, but libc++ does not implement those. The float and bool implementations of YamlParser::Getter::get() are therefore kept as-is. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/internal/yaml_parser.h | 2 +- src/libcamera/yaml_parser.cpp | 168 ++++------------------- 2 files changed, 29 insertions(+), 141 deletions(-) base-commit: dcb90f13cf79d909d04dacf2cfbd256a4744347f