[libcamera-devel] [PATCH v1 2/2] libcamera: base: utils: Support C libraries lacking locale support

[Laurent Pinchart]

Hi Jacopo,

On Tue, Jan 10, 2023 at 09:13:53AM +0100, Jacopo Mondi wrote:
> On Sun, Jan 08, 2023 at 11:43:57PM +0200, Laurent Pinchart via libcamera-devel wrote:
> > Not all C libraries include support for locale objects (locale_t) and
> > the strto*_l() family of functions. A notable example is uClibc that can
> > be compiled with a hardcoded "C" locale. Compilation then fails as the
> > newlocale(), freelocale() and strtod_l() functions are not defined.
> >
> > Fix the compilation breakage by checking for the availability of
> > newlocale(), and fall back to strtod() when the function isn't
> > available. This may not lead to the correct result if support for locale
> > objects isn't available and the locale isn't hardcoded to "C", but that
> > is such a corner case that we will likely never encounter it.
> >
> > Fixes: e8ae254970cf ("libcamera: yaml_parser: Use C locale")
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  meson.build                  |  4 ++++
> >  src/libcamera/base/utils.cpp | 12 ++++++++++++
> >  2 files changed, 16 insertions(+)
> >
> > diff --git a/meson.build b/meson.build
> > index e86673dd5c0c..7dcd34d3e6b3 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -72,6 +72,10 @@ if cc.has_header_symbol('unistd.h', 'issetugid')
> >      config_h.set('HAVE_ISSETUGID', 1)
> >  endif
> >
> > +if cc.has_header_symbol('locale.h', 'newlocale', prefix : '#define _GNU_SOURCE')
> > +    config_h.set('HAVE_NEWLOCALE', 1)
> > +endif
> > +
> >  if cc.has_header_symbol('stdlib.h', 'secure_getenv', prefix : '#define _GNU_SOURCE')
> >      config_h.set('HAVE_SECURE_GETENV', 1)
> >  endif
> > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp
> > index 4a239427a4d9..ac3e1311995e 100644
> > --- a/src/libcamera/base/utils.cpp
> > +++ b/src/libcamera/base/utils.cpp
> > @@ -464,6 +464,8 @@ std::string toAscii(const std::string &str)
> >   * \a b
> >   */
> >
> > +#if HAVE_NEWLOCALE
> > +
> 
> Just bikeshedding on the NEWLOCALE symbol. "New" to implies there's an
> old vs new version, while you're just here checking that locale is
> supported. Maybe just HAVE_LOCALE ?

It comes directly from the name of the function being checked in
meson.build, newlocale(). I could replace that with a check for the
locale_t type and use HAVE_LOCALE_T if preferred.

> Apart from this:
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> 
> >  namespace {
> >
> >  /*
> > @@ -493,6 +495,8 @@ Locale cLocale("C");
> >
> >  } /* namespace */
> >
> > +#endif /* HAVE_NEWLOCALE */
> > +
> >  /**
> >   * \brief Convert a string to a double independently of the current locale
> >   * \param[in] nptr The string to convert
> > @@ -506,7 +510,15 @@ Locale cLocale("C");
> >   */
> >  double strtod(const char *__restrict nptr, char **__restrict endptr)
> >  {
> > +#if HAVE_NEWLOCALE
> >  	return strtod_l(nptr, endptr, cLocale.locale());
> > +#else
> > +	/*
> > +	 * If the libc implementation doesn't provide locale object support,
> > +	 * assume that strtod() is locale-independent.
> > +	 */
> > +	return strtod(nptr, endptr);
> > +#endif
> >  }
> >
> >  } /* namespace utils */

-- 
Regards,

Laurent Pinchart