[libcamera-devel] [PATCH v4 6/9] libcamera: yaml_parser: De-duplicate common code in YamlObject::get()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Aug 18 19:41:25 CEST 2022
Hi Jacopo,
On Thu, Aug 18, 2022 at 03:44:34PM +0200, Jacopo Mondi wrote:
> 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.
I thought about it and then decided to leave it for later, but I think I
can already change this to use a long (and an unsigned long below) as
those are the return types of strtol and strtoul. It won't give us
64-bit support on 32-bit platforms, that will need to be handled when
adding 64-bit integer support in any case.
> For now I think it's fine
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> > +{
> > + 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
More information about the libcamera-devel
mailing list