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

2015-03-16 Thread Erik Faye-Lund 
On Tue, Mar 17, 2015 at 12:32 AM, Ian Romanick i...@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 kusmab...@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

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

2015-03-16 Thread Matt Turner 
On Sun, Mar 15, 2015 at 12:05 PM, Erik Faye-Lund kusmab...@gmail.com wrote:
 _mesa_strtod and _mesa_strtof are only used from the GLSL compiler,
 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.

 Signed-off-by: Erik Faye-Lund kusmab...@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++.

This looks good to me. It'd be nice if idr could take a look as well.

Reviewed-by: Matt Turner matts...@gmail.com

The one annoyance is that autotools doesn't work across this change.
After applying this patch to a configured and built tree, you'll get

make[4]: *** No rule to make target
'../../../mesa/src/util/strtod.cpp', needed by
'libmesautil_la-strtod.lo'.  Stop.

when running make.

To fix, run

sed -i -e 's/strtod.cpp/strtod.c/' src/util/.deps/libmesautil_la-strtod.Plo

The file will be in your build tree.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-03-16 Thread Emil Velikov 
On 15/03/15 19:05, Erik Faye-Lund wrote:
 _mesa_strtod and _mesa_strtof are only used from the GLSL compiler,
 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.
 
 Signed-off-by: Erik Faye-Lund kusmab...@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++.
 
Fwiw this file/code should not cause any linkage to the C++ runtime,
although it's a nice cleanup imho.

There is a small catch though - _mesa_strtof can be used by the
standalone glsl_compiler and perhaps glcpp. I could not find any
references in the manpages about locale_t's implementation although it
guaranteed to be a struct for every platform we can just add an assert
in _mesa_strtof and fix the fallouts later ?

-Emil

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-03-16 Thread Erik Faye-Lund 
On Mon, Mar 16, 2015 at 10:13 PM, Emil Velikov emil.l.veli...@gmail.com wrote:
 On 15/03/15 19:05, Erik Faye-Lund wrote:
 _mesa_strtod and _mesa_strtof are only used from the GLSL compiler,
 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.

 Signed-off-by: Erik Faye-Lund kusmab...@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++.

 Fwiw this file/code should not cause any linkage to the C++ runtime,
 although it's a nice cleanup imho.

 There is a small catch though - _mesa_strtof can be used by the
 standalone glsl_compiler and perhaps glcpp.

Good point, so perhaps this on top?

---8---
diff --git a/src/glsl/glcpp/glcpp.c b/src/glsl/glcpp/glcpp.c
index 5144516..c62f4ef 100644
--- a/src/glsl/glcpp/glcpp.c
+++ b/src/glsl/glcpp/glcpp.c
@@ -29,6 +29,7 @@
 #include glcpp.h
 #include main/mtypes.h
 #include main/shaderobj.h
+#include util/strtod.h

 extern int glcpp_parser_debug;

@@ -168,6 +169,8 @@ main (int argc, char *argv[])
  if (shader == NULL)
return 1;

+ _mesa_locale_init();
+
  ret = glcpp_preprocess(ctx, shader, info_log, NULL, gl_ctx);

  printf(%s, shader);
diff --git a/src/glsl/main.cpp b/src/glsl/main.cpp
index ccac839..b23b583 100644
--- a/src/glsl/main.cpp
+++ b/src/glsl/main.cpp
@@ -38,6 +38,7 @@
 #include program/hash_table.h
 #include loop_analysis.h
 #include standalone_scaffolding.h
+#include util/strtod.h

 static int glsl_version = 330;

@@ -52,6 +53,8 @@ initialize_context(struct gl_context *ctx, gl_api api)
 {
initialize_context_to_defaults(ctx, api);

+   _mesa_locale_init();
+
/* The standalone compiler needs to claim support for almost
 * everything in order to compile the built-in functions.
 */
---8---
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-03-16 Thread Ian Romanick 
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.

 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

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.

It also occurs to me that the neither the old code nor the new code ever
call freelocale.  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.

 Signed-off-by: Erik Faye-Lund kusmab...@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);
  

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-03-15 Thread Erik Faye-Lund 
_mesa_strtod and _mesa_strtof are only used from the GLSL compiler,
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.

Signed-off-by: Erik Faye-Lund kusmab...@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);
 
-- 
2.1.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev