| Message ID | 20230108214357.12641-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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@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 >
Hi Laurent I presume set_locale() https://en.cppreference.com/w/cpp/locale/setlocale doesn't help here, right ? On Sun, Jan 08, 2023 at 11:43:56PM +0200, Laurent Pinchart via libcamera-devel wrote: > 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@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. > + */ > +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 >
Hi Jacopo, On Tue, Jan 10, 2023 at 09:10:52AM +0100, Jacopo Mondi wrote: > Hi Laurent > > I presume set_locale() > https://en.cppreference.com/w/cpp/locale/setlocale > > doesn't help here, right ? It doesn't, as that changes the current locale, which we can't do as a library. > On Sun, Jan 08, 2023 at 11:43:56PM +0200, Laurent Pinchart via libcamera-devel wrote: > > 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@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. > > + */ > > +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;
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. + */ +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;
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@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(-)