Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mNumericLocale only considers C-locale, but not C++-locale #132

Closed
mejdys opened this issue Feb 20, 2023 · 2 comments
Closed

mNumericLocale only considers C-locale, but not C++-locale #132

mejdys opened this issue Feb 20, 2023 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@mejdys
Copy link

mejdys commented Feb 20, 2023

Description:

Setting mNumericLocale to false, switches the number parsing from stod to istringstream's operator>>. However, this just switches to a different locale, and does not disable it entirely.

It comes down to stod parsing as per locale set with the C routine setlocale and istringstream - as per locale set with C++ routine std::locale::global. So, the flag does not disable locale, but switches from the locale set in the C world, to the one set in the C++ world. Annoying for all of us, these can be different, not least because calling setlocale does not change std::locale::global

How to reproduce it:

With this patch to test087.cpp:

diff --git a/tests/test087.cpp b/tests/test087.cpp
index 5d8085a..0372b2b 100644
--- a/tests/test087.cpp
+++ b/tests/test087.cpp
@@ -16,6 +16,7 @@ int main()
     // ensures that the necessary locale is installed.
     return 0;
   }
+  std::locale::global(std::locale("de_DE"));
 
   std::string path = unittest::TempPath();
 

The test will start failing:

% ./test087 
istringstream: no conversion

Environment:

  • Version: main@78626c699990abda8b5dc4404d1916239584cf91
  • OS / distro: macOS
@mejdys mejdys added the bug Something isn't working label Feb 20, 2023
@d99kris
Copy link
Owner

d99kris commented Feb 20, 2023

Hi @mejdys - thanks for reporting this and providing a detailed bug report!

I'll look into adding a separate test case and fix for this within the next week. If you need a quick local fix for this I believe something like the following patch would fix it:

diff --git a/src/rapidcsv.h b/src/rapidcsv.h
index 01fbb37..2b82b0e 100644
--- a/src/rapidcsv.h
+++ b/src/rapidcsv.h
@@ -247,6 +247,7 @@ namespace rapidcsv
               (typeid(T) == typeid(long double)))
           {
             std::istringstream iss(pStr);
+            iss.imbue(std::locale::classic());
             iss >> pVal;
             if (iss.fail() || iss.bad() || !iss.eof())
             {

@d99kris
Copy link
Owner

d99kris commented Feb 25, 2023

Hi again, the mentioned fix has been committed now, along with a separate test case for it. Let me know if you encounter any issues. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants