[PATCH] libcamera: yaml_parser: Use std::from_chars()

[Jacopo Mondi]

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 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>

Thanks
  j

> --
> Regards,
>
> Laurent Pinchart
>