[libcamera-devel] [PATCH v1 1/2] base: utils: Add and use strtod() helper

[Kieran Bingham]

Quoting Laurent Pinchart via libcamera-devel (2023-01-08 21:43:56)
> The strtod() function is locale-dependent, and thus ill-suited to parse
> numbers coming from, for instance, YAML files. The YamlObject class uses
> strtod_l() to fix that issue, but that function is not available with
> all libc implementations. Correctly handling this problem is becoming
> out of scope for the YamlObject class.
> 
> As a first step, add a strtod() helper function in the utils namespace
> that copies the implementation from YamlObject, and use it in
> YamlObject. The core issue will then be fixed in utils::strtod().
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/base/utils.h |  2 ++
>  src/libcamera/base/utils.cpp   | 46 ++++++++++++++++++++++++++++++++++
>  src/libcamera/yaml_parser.cpp  | 34 +------------------------
>  3 files changed, 49 insertions(+), 33 deletions(-)
> 
> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> index eb7bcdf4c173..37d9af609ec7 100644
> --- a/include/libcamera/base/utils.h
> +++ b/include/libcamera/base/utils.h
> @@ -367,6 +367,8 @@ decltype(auto) abs_diff(const T &a, const T &b)
>                 return a - b;
>  }
>  
> +double strtod(const char *__restrict nptr, char **__restrict endptr);
> +
>  } /* namespace utils */
>  
>  #ifndef __DOXYGEN__
> diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp
> index 6a307940448e..4a239427a4d9 100644
> --- a/src/libcamera/base/utils.cpp
> +++ b/src/libcamera/base/utils.cpp
> @@ -8,6 +8,7 @@
>  #include <libcamera/base/utils.h>
>  
>  #include <iomanip>
> +#include <locale.h>
>  #include <sstream>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -463,6 +464,51 @@ std::string toAscii(const std::string &str)
>   * \a b
>   */
>  
> +namespace {
> +
> +/*
> + * RAII wrapper around locale_t instances, to support global locale instances
> + * without leaking memory.
> + */
> +class Locale
> +{
> +public:
> +       Locale(const char *locale)
> +       {
> +               locale_ = newlocale(LC_ALL_MASK, locale, static_cast<locale_t>(0));
> +       }
> +
> +       ~Locale()
> +       {
> +               freelocale(locale_);
> +       }
> +
> +       locale_t locale() { return locale_; }
> +
> +private:
> +       locale_t locale_;
> +};
> +
> +Locale cLocale("C");
> +
> +} /* namespace */
> +
> +/**
> + * \brief Convert a string to a double independently of the current locale
> + * \param[in] nptr The string to convert
> + * \param[out] endptr Pointer to trailing portion of the string after conversion
> + *
> + * This function is a locale-independent version of the std::strtod() function.
> + * It behaves as the standard function, but uses the "C" locale instead of the
> + * current locale.
> + *
> + * \return The converted value, if any, or 0.0 if the conversion failed.
> + */

Sounds good.


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +double strtod(const char *__restrict nptr, char **__restrict endptr)
> +{
> +       return strtod_l(nptr, endptr, cLocale.locale());
> +}
> +
>  } /* namespace utils */
>  
>  #ifndef __DOXYGEN__
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index 2806c591f75d..153a6d53c3f9 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -31,38 +31,6 @@ namespace {
>  /* Empty static YamlObject as a safe result for invalid operations */
>  static const YamlObject empty;
>  
> -/*
> - * Construct a global RAII locale for use by all YAML parser instances to
> - * ensure consistency when parsing configuration files and types regardless of
> - * the system locale configuration.
> - *
> - * For more information see:
> - * - https://bugs.libcamera.org/show_bug.cgi?id=174
> - */
> -class Locale
> -{
> -public:
> -       Locale(const char *locale)
> -       {
> -               locale_ = newlocale(LC_ALL_MASK, locale, static_cast<locale_t>(0));
> -               if (locale_ == static_cast<locale_t>(0))
> -                       LOG(YamlParser, Fatal)
> -                               << "Failed to construct a locale";
> -       }
> -
> -       ~Locale()
> -       {
> -               freelocale(locale_);
> -       }
> -
> -       locale_t locale() { return locale_; }
> -
> -private:
> -       locale_t locale_;
> -};
> -
> -Locale yamlLocale("C");
> -
>  } /* namespace */
>  
>  /**
> @@ -315,7 +283,7 @@ std::optional<double> YamlObject::get() const
>         char *end;
>  
>         errno = 0;
> -       double value = strtod_l(value_.c_str(), &end, yamlLocale.locale());
> +       double value = utils::strtod(value_.c_str(), &end);
>  
>         if ('\0' != *end || errno == ERANGE)
>                 return std::nullopt;
> -- 
> Regards,
> 
> Laurent Pinchart
>