[PATCH] libcamera: yaml_parser: Use std::from_chars()
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Nov 13 12:49:48 CET 2024
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 at ideasonboard.com>
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at 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 at ideasonboard.com>
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list