asmaloney / libE57Format

Library for reading & writing the E57 file format
Boost Software License 1.0
137 stars 66 forks source link

Floating point number parsing is affected by the locale #172

Closed Hugal31 closed 2 years ago

Hugal31 commented 2 years ago

If I have a program using this library while having the locale set to a language using for example commas as the decimal separator, the numbers are parsed incorrectly.

For example, 8.123e-2 becomes simply 8.

asmaloney commented 2 years ago

What did you use to write the E57 file you're working with? It isn't following the E57 standard - the bug is on write, not read.

8.3.4.2 The value of a Float is represented as child text of the XML element. This child text shall be zero or one occurrence of the xsd:float representation (if precision is single) or xsd:double (if precision is double), with optional leading and trailing XML whitespace.

xs:float:

The decimal separator is always a point (“.”) and no thousands separator may be added.

If you wrote the file using libE57Format - then the bug is with writing! 😄

Looking at the libE57Format code, it might be possible to write these out incorrectly if setting std::locale::global directly. I'll need to do some more testing.

asmaloney commented 2 years ago

It is possible to write out invalid E57 files if you set std::locale::global directly.

Fix incoming...

Hugal31 commented 2 years ago

I thinkg the writing is working correctly (you even have a custom function to write floats). I am talking about reading a standard E57 file. The file contains regular "english" floats with dots, but when the locale is set to french for instance, atofwaits for a comma instead of a dot:

#include <cstdlib>
#include <iostream>
#include <locale>

int main()
{
    std::setlocale(LC_ALL, "fr_FR.UTF-8");
    float foo = atof("1.234");
    std::cout << foo; // Display "1"
}
asmaloney commented 2 years ago

Ah! Got it. I thought you were saying the E57 file had commas in it which was causing a read failure.

I guess the original authors expected everyone to set their locale to "C" 😄 I'm surprised this hasn't come up before.

Incidentally, the writing is not working correctly either if you set std::locale::global!

Try this test:

TEST( StringFunctions, FloatToStrLocale )
{
   std::locale::global( std::locale( "fr_FR" ) );
   std::wcout.imbue( std::locale() );

   const auto converted = e57::floatingPointToStr<float>( 123456.0f, 7 );

   ASSERT_EQ( converted, "1.2345600e+05" );

   std::locale::global( std::locale::classic() );
}
Hugal31 commented 2 years ago

Indeed, I miss-understood how floatingPointToStr works. I'll add a call to imbue to my PR.

asmaloney commented 2 years ago

I've already got it done.

(Edit: Except for the CI problems of course 😆 )