DoubleToStringConverter::EcmaScriptConverter() not thread safe on Windows

[floitsch]

From http://code.google.com/p/double-conversion/issues/detail?id=30 :

-- sean.parent
Code inspection at: double-conversion.cc:44

Function level statics are not initialized in a thread safe manner with Visual Studio 2012 or prior. With gcc and clang thread safe initialization is optional (though required by C++11).

Could be fixed with any number of once init mechanisms, or aggregate initialization.

-- floitsch
I don't see any easy way of solving this without pulling in thread-specific headers.
I would rather just require -std=c++0x which, afaik, guarantees correct initialization.
I will discuss this with my colleagues next week and see if somebody comes up with a better solution.

-- stephen.mercer
If you simply move the function static out of the function and make it static to the file then it will be thread safe initialized. Reference this answer:
http://stackoverflow.com/questions/1962880/is-c-static-member-variable-initialization-thread-safe

So to make it thread safe, simply replace this code (and similar if you have other function-level statics):

const DoubleToStringConverter& DoubleToStringConverter::EcmaScriptConverter() {
   int flags = UNIQUE_ZERO | EMIT_POSITIVE_EXPONENT_SIGN;
   static DoubleToStringConverter converter(flags,
                                           "Infinity",
                                           "NaN",
                                           'e',
                                           -6, 21,
                                           6, 0);
  return converter;
}

with this code:

static DoubleToStringConverter kConverter(UNIQUE_ZERO | EMIT_POSITIVE_EXPONENT_SIGN,
                                          "Infinity",
                                          "NaN",
                                          'e',
                                          -6, 21,
                                          6, 0);

const DoubleToStringConverter& DoubleToStringConverter::EcmaScriptConverter() {
  return kConverter;
}

-- rafaelefernandez
You definitely do not want to move it out of the function because then you will run into the static initialization order problem.

[yumeyao]

how about making the variable const and the constructor constexpr(c++11 or above), it's straight-forward.

class DoubleToStringConverter;
extern const DoubleToStringConverter ecmaScriptConverter;

class DoubleToStringConverter {
constexpr DoubleToStringConverter(...) {...}
//.....
static const DoubleToStringConverter& EcmaScriptConverter() { return ecmaScriptConverter; }
};

const DoubleToStringConverter ecmaScriptConverter = DoubleToStringConverter (....);

For old C++ standard, we can take advantage of POD struct initialization, but for doing so we'll have to break some of the existing code using this library or do some hackish work.

in header file

struct InternalStruct {
//no constructor

//fields
int flags; //no const
//....
};

extern const InternalStruct ecmaScriptConverter;

class DoubleToStringConverter : InternalStruct {
DoubleToStringConverter(...) {
//have to not use initialization list for no support on member aggregate initialization in old c++ std
this->flags = flags;
//...assign other fields
ASSERT(assert_something);
}
}

in cc file
const InternalStruct ecmaScriptConverter = {9, "Infinity", "NaN", 'e', -6, 21, 6, 0};

Now we have 2 ways:

1. breaking some existing code
   Declare all functions(except constructor) in InternalStruct, and declare EcmaScriptConverter() to return const InternalStruct& instead of const DoubleToStringConverter&, i.e.
   static const InternalStruct& EcmaScriptConverter();
   By doing so, we break code that uses

   const DoubleToStringConverter& cvtr=DoubleToStringConverter::EcmaScriptConverter();
   cvtr.doSomething();
   cvtr.doSomethingElse();
   

2. hackish way to cast InternalStruct to DoubleToStringConverter. casting a base obj to derived ref should not be allowed.

   static const DoubleToStringConverter& EcmaScriptConverter() { return static_cast<const   DoubleToStringConverter&>(ecmaScriptConverter); }
   

So no perfect solution on below c++11. But at least, we can use macros to give users have c++11 enabled a faster & better implementation. (no static initialization issue, no multi-thread issue, also we can mark EcmaScriptConverter() as inline to return the extern obj)

[mihaipopescu]

We should try not to change the minimum supported C++ standard as it will reduce the compatibility for compiling this library on various platforms. For instance, compilers for N3DS or WiiU are stuck with old versions and they are not going to upgrade any time soon.

You definitely do not want to move it out of the function because then you will run into the static initialization order problem.

I totally agree with this.

hackish way to cast InternalStruct to DoubleToStringConverter. casting a base obj to derived ref should not be allowed.

For that particular solution you also need a converting constructor and it could work... but it's rather complex I have to admit.

For this particular case, another solution would just remove static const DoubleToStringConverter::DoubleToStringConverter& EcmaScriptConverter(); from the class.

If we're concerned about breaking public API then we can provide a singleton implementation. What do you guys think ?

[yumeyao]

@mihaipopescu

We should try not to change the minimum supported C++ standard as it will reduce the compatibility for compiling this library on various platforms. For instance, compilers for N3DS or WiiU are stuck with old versions and they are not going to upgrade any time soon.

That's why I said

But at least, we can use macros to give users have c++11 enabled a faster & better implementation.

generally the c++11 solution seems perfect (needs some special handling of the ASSERT inside the constructor), and we can fall back to singleton/static implementation for old c++.

[mihaipopescu]

we can fall back to singleton/static implementation for old c++

👍

[floitsch]

What, if we just added the flag, when the compiler supports it?

I just added a conditional flag for CMake (-fPIC if supported).

The following link has similar code for adding the c++0x flag: http://stackoverflow.com/questions/25451254/getting-cmake-check-cxx-compiler-flag-to-work

I could try to find a similar pattern for scons, and we should hopefully be able to get something similar for bazel. @tfarina what do you think?

[tfarina]

@floitsch I would prefer to avoid introducing any hack in the C++ code if possible.

If we can, then I can help to look at doing the changes for the Bazel build.