[libcamera-devel] [PATCH v4 6/9] libcamera: yaml_parser: De-duplicate common code in YamlObject::get()

[Jacopo Mondi]

Hi Laurent

On Tue, Aug 16, 2022 at 04:54:11AM +0300, Laurent Pinchart via libcamera-devel wrote:
> The specializations of the YamlObject::get() function template for
> integer types duplicate code that doesn't directly depend on the
> template type argument. Move it to separate helper functions to reduce
> the object size.
>
> While at it, rephrase the comment about unsigned integer parsing.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/yaml_parser.cpp | 160 ++++++++++++++--------------------
>  1 file changed, 67 insertions(+), 93 deletions(-)
>
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index 85a52c05e682..3d5efdc4419b 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -131,23 +131,65 @@ std::optional<bool> YamlObject::get() const
>  	return std::nullopt;
>  }
>
> +namespace {
> +
> +bool parseSignedInteger(const std::string &str, int32_t min, int32_t max,
> +			int32_t *result)

The usage of int32_t types might not scale if we want to add support
for parsing 64-bit values.

For now I think it's fine
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

> +{
> +	if (str == "")
> +		return false;
> +
> +	char *end;
> +
> +	errno = 0;
> +	long value = std::strtol(str.c_str(), &end, 10);
> +
> +	if ('\0' != *end || errno == ERANGE || value < min || value > max)
> +		return false;
> +
> +	*result = value;
> +	return true;
> +}
> +
> +bool parseUnsignedInteger(const std::string &str, uint32_t max, uint32_t *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::get() const
>  {
>  	if (type_ != Type::Value)
>  		return std::nullopt;
>
> -	if (value_ == "")
> -		return std::nullopt;
> +	int32_t value;
>
> -	char *end;
> -
> -	errno = 0;
> -	long value = std::strtol(value_.c_str(), &end, 10);
> -
> -	if ('\0' != *end || errno == ERANGE ||
> -	    value < std::numeric_limits<int8_t>::min() ||
> -	    value > std::numeric_limits<int8_t>::max())
> +	if (!parseSignedInteger(value_, std::numeric_limits<int8_t>::min(),
> +				std::numeric_limits<int8_t>::max(), &value))
>  		return std::nullopt;
>
>  	return value;
> @@ -159,28 +201,10 @@ std::optional<uint8_t> YamlObject::get() const
>  	if (type_ != Type::Value)
>  		return std::nullopt;
>
> -	if (value_ == "")
> -		return std::nullopt;
> +	uint32_t value;
>
> -	/*
> -	 * libyaml parses all scalar values as strings. When a string has
> -	 * leading spaces before a minus sign, for example "  -10", strtoul
> -	 * skips leading spaces, accepts the leading minus sign, and the
> -	 * calculated digits are negated as if by unary minus. Rule it out in
> -	 * case the user gets a large number when the value is negative.
> -	 */
> -	std::size_t found = value_.find_first_not_of(" \t");
> -	if (found != std::string::npos && value_[found] == '-')
> -		return std::nullopt;
> -
> -	char *end;
> -
> -	errno = 0;
> -	unsigned long value = std::strtoul(value_.c_str(), &end, 10);
> -
> -	if ('\0' != *end || errno == ERANGE ||
> -	    value < std::numeric_limits<uint8_t>::min() ||
> -	    value > std::numeric_limits<uint8_t>::max())
> +	if (!parseUnsignedInteger(value_, std::numeric_limits<uint8_t>::max(),
> +				  &value))
>  		return std::nullopt;
>
>  	return value;
> @@ -192,17 +216,10 @@ std::optional<int16_t> YamlObject::get() const
>  	if (type_ != Type::Value)
>  		return std::nullopt;
>
> -	if (value_ == "")
> -		return std::nullopt;
> +	int32_t value;
>
> -	char *end;
> -
> -	errno = 0;
> -	long value = std::strtol(value_.c_str(), &end, 10);
> -
> -	if ('\0' != *end || errno == ERANGE ||
> -	    value < std::numeric_limits<int16_t>::min() ||
> -	    value > std::numeric_limits<int16_t>::max())
> +	if (!parseSignedInteger(value_, std::numeric_limits<int16_t>::min(),
> +				std::numeric_limits<int16_t>::max(), &value))
>  		return std::nullopt;
>
>  	return value;
> @@ -214,28 +231,10 @@ std::optional<uint16_t> YamlObject::get() const
>  	if (type_ != Type::Value)
>  		return std::nullopt;
>
> -	if (value_ == "")
> -		return std::nullopt;
> +	uint32_t value;
>
> -	/*
> -	 * libyaml parses all scalar values as strings. When a string has
> -	 * leading spaces before a minus sign, for example "  -10", strtoul
> -	 * skips leading spaces, accepts the leading minus sign, and the
> -	 * calculated digits are negated as if by unary minus. Rule it out in
> -	 * case the user gets a large number when the value is negative.
> -	 */
> -	std::size_t found = value_.find_first_not_of(" \t");
> -	if (found != std::string::npos && value_[found] == '-')
> -		return std::nullopt;
> -
> -	char *end;
> -
> -	errno = 0;
> -	unsigned long value = std::strtoul(value_.c_str(), &end, 10);
> -
> -	if ('\0' != *end || errno == ERANGE ||
> -	    value < std::numeric_limits<uint16_t>::min() ||
> -	    value > std::numeric_limits<uint16_t>::max())
> +	if (!parseUnsignedInteger(value_, std::numeric_limits<uint16_t>::max(),
> +				  &value))
>  		return std::nullopt;
>
>  	return value;
> @@ -247,17 +246,10 @@ std::optional<int32_t> YamlObject::get() const
>  	if (type_ != Type::Value)
>  		return std::nullopt;
>
> -	if (value_ == "")
> -		return std::nullopt;
> +	int32_t value;
>
> -	char *end;
> -
> -	errno = 0;
> -	long value = std::strtol(value_.c_str(), &end, 10);
> -
> -	if ('\0' != *end || errno == ERANGE ||
> -	    value < std::numeric_limits<int32_t>::min() ||
> -	    value > std::numeric_limits<int32_t>::max())
> +	if (!parseSignedInteger(value_, std::numeric_limits<int32_t>::min(),
> +				std::numeric_limits<int32_t>::max(), &value))
>  		return std::nullopt;
>
>  	return value;
> @@ -269,28 +261,10 @@ std::optional<uint32_t> YamlObject::get() const
>  	if (type_ != Type::Value)
>  		return std::nullopt;
>
> -	if (value_ == "")
> -		return std::nullopt;
> +	uint32_t value;
>
> -	/*
> -	 * libyaml parses all scalar values as strings. When a string has
> -	 * leading spaces before a minus sign, for example "  -10", strtoul
> -	 * skips leading spaces, accepts the leading minus sign, and the
> -	 * calculated digits are negated as if by unary minus. Rule it out in
> -	 * case the user gets a large number when the value is negative.
> -	 */
> -	std::size_t found = value_.find_first_not_of(" \t");
> -	if (found != std::string::npos && value_[found] == '-')
> -		return std::nullopt;
> -
> -	char *end;
> -
> -	errno = 0;
> -	unsigned long value = std::strtoul(value_.c_str(), &end, 10);
> -
> -	if ('\0' != *end || errno == ERANGE ||
> -	    value < std::numeric_limits<uint32_t>::min() ||
> -	    value > std::numeric_limits<uint32_t>::max())
> +	if (!parseUnsignedInteger(value_, std::numeric_limits<uint32_t>::max(),
> +				  &value))
>  		return std::nullopt;
>
>  	return value;
> --
> Regards,
>
> Laurent Pinchart
>