libcamera: yaml_parser: Use std::from_chars()
diff mbox series

Message ID 20241112185637.10232-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit 5c71df927ddaaa01204bff1e647c9d2bf653d95f
Headers show
Series
  • libcamera: yaml_parser: Use std::from_chars()
Related show

Commit Message

Laurent Pinchart Nov. 12, 2024, 6:56 p.m. UTC
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

Comments

Jacopo Mondi Nov. 13, 2024, 11:39 a.m. UTC | #1
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
>
Laurent Pinchart Nov. 13, 2024, 11:43 a.m. UTC | #2
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>
Kieran Bingham Nov. 13, 2024, 11:49 a.m. UTC | #3
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

Patch
diff mbox series

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>