[Mesa-dev] [PATCH] util: port _mesa_strto[df] to C

[Erik Faye-Lund]

On Tue, Mar 17, 2015 at 12:32 AM, Ian Romanick <idr at freedesktop.org> wrote:
> On 03/15/2015 12:05 PM, Erik Faye-Lund wrote:
>> _mesa_strtod and _mesa_strtof are only used from the GLSL compiler,
>
> It's also used in the ARB_vertex_program / ARB_fragment_program
> assembler in src/prog.

Oh, right. Thanks for pointing that out.

>> so the locale doesn't need to be initialized before the first context
>> gets initialized. So let's use explicit initialization from the
>> one-time init code instead of depending on a C++ compiler to initialize
>> at image-load time.
>
> This is fairly close to the way Chia-I originally had it:
>
> http://lists.freedesktop.org/archives/mesa-dev/2014-April/058215.html
>
> Some discussion of alternate methods started:
>
> http://lists.freedesktop.org/archives/mesa-dev/2014-May/058861.html

Thanks for pointing me at this discussion, very useful.

> I'm a little concerned that having the initialization in Mesa and the
> function accessible to both Mesa and Gallium that we may set ourselves
> up for problems later.

Doesn't really sound like a ground-shattering risk to me. But perhaps
adding an assert verifying that initialization was done could offset
that risk?

> It also occurs to me that the neither the old code nor the new code ever
> call freelocale.

In OpenGL ES 2.0 and OpenGL 4.1, you have glReleaseShaderCompiler
which is intended for this kind of work. But I'm not sure a single
leak of a locale is really worth the implementation-effort.

> I think that's easier to fix with the static object
> method (using a destructor).
>
> I guess I'm kind of ambivalent about the change.

Yeah, especially initialization having to be done in three different
locations causes me to start losing some confidence that this is a
good idea.

>> Signed-off-by: Erik Faye-Lund <kusmabite at gmail.com>
>> ---
>>
>> Because of the recent discussion on libc++ and Mesa, I thought I'd
>> have a look into what parts of mesa depended on libc++, and I spotted
>> this file.
>>
>> In this case, it was rather trivial to port the code to plain C, making
>> it dead obvious that it doesn't depend on libc++. I'm not proposing all
>> C++ gets this treatment, but in this case it seems like a pretty
>> straight-forward way to make it obvious that this code does not depend
>> on libc++.
>>
>>  src/mesa/main/context.c           |  3 +++
>>  src/util/Makefile.sources         |  2 +-
>>  src/util/{strtod.cpp => strtod.c} | 14 ++++++++------
>>  src/util/strtod.h                 |  3 +++
>>  4 files changed, 15 insertions(+), 7 deletions(-)
>>  rename src/util/{strtod.cpp => strtod.c} (89%)
>>
>> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
>> index 22c2341..de6a016 100644
>> --- a/src/mesa/main/context.c
>> +++ b/src/mesa/main/context.c
>> @@ -119,6 +119,7 @@
>>  #include "shared.h"
>>  #include "shaderobj.h"
>>  #include "util/simple_list.h"
>> +#include "util/strtod.h"
>>  #include "state.h"
>>  #include "stencil.h"
>>  #include "texcompress_s3tc.h"
>> @@ -398,6 +399,8 @@ one_time_init( struct gl_context *ctx )
>>        assert( sizeof(GLint) == 4 );
>>        assert( sizeof(GLuint) == 4 );
>>
>> +      _mesa_locale_init();
>> +
>>        _mesa_one_time_init_extension_overrides();
>>
>>        _mesa_get_cpu_features();
>> diff --git a/src/util/Makefile.sources b/src/util/Makefile.sources
>> index 560ea83..f930790 100644
>> --- a/src/util/Makefile.sources
>> +++ b/src/util/Makefile.sources
>> @@ -17,7 +17,7 @@ MESA_UTIL_FILES :=  \
>>       set.c \
>>       set.h \
>>       simple_list.h \
>> -     strtod.cpp \
>> +     strtod.c \
>>       strtod.h \
>>       texcompress_rgtc_tmp.h \
>>       u_atomic.h
>> diff --git a/src/util/strtod.cpp b/src/util/strtod.c
>> similarity index 89%
>> rename from src/util/strtod.cpp
>> rename to src/util/strtod.c
>> index 2b4dd98..a4a60e0 100644
>> --- a/src/util/strtod.cpp
>> +++ b/src/util/strtod.c
>> @@ -30,18 +30,20 @@
>>  #include <locale.h>
>>  #ifdef HAVE_XLOCALE_H
>>  #include <xlocale.h>
>> +static locale_t loc;
>>  #endif
>>  #endif
>>
>>  #include "strtod.h"
>>
>>
>> +void
>> +_mesa_locale_init(void)
>> +{
>>  #if defined(_GNU_SOURCE) && defined(HAVE_XLOCALE_H)
>> -static struct locale_initializer {
>> -   locale_initializer() { loc = newlocale(LC_CTYPE_MASK, "C", NULL); }
>> -   locale_t loc;
>> -} loc_init;
>> +   loc = newlocale(LC_CTYPE_MASK, "C", NULL);
>>  #endif
>> +}
>>
>>  /**
>>   * Wrapper around strtod which uses the "C" locale so the decimal
>> @@ -51,7 +53,7 @@ double
>>  _mesa_strtod(const char *s, char **end)
>>  {
>>  #if defined(_GNU_SOURCE) && defined(HAVE_XLOCALE_H)
>> -   return strtod_l(s, end, loc_init.loc);
>> +   return strtod_l(s, end, loc);
>>  #else
>>     return strtod(s, end);
>>  #endif
>> @@ -66,7 +68,7 @@ float
>>  _mesa_strtof(const char *s, char **end)
>>  {
>>  #if defined(_GNU_SOURCE) && defined(HAVE_XLOCALE_H)
>> -   return strtof_l(s, end, loc_init.loc);
>> +   return strtof_l(s, end, loc);
>>  #elif defined(HAVE_STRTOF)
>>     return strtof(s, end);
>>  #else
>> diff --git a/src/util/strtod.h b/src/util/strtod.h
>> index 02c25dd..b7e2beb 100644
>> --- a/src/util/strtod.h
>> +++ b/src/util/strtod.h
>> @@ -31,6 +31,9 @@
>>  extern "C" {
>>  #endif
>>
>> +extern void
>> +_mesa_locale_init(void);
>> +
>>  extern double
>>  _mesa_strtod(const char *s, char **end);
>>
>