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.

Sign up for GitHub

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

Jump to bottom

parsing of doubles depends on locale #1566

Open
lkeegan opened this issue Jul 18, 2019 · 2 comments · May be fixed by #1567 or #1993
Open

parsing of doubles depends on locale #1566

lkeegan opened this issue Jul 18, 2019 · 2 comments · May be fixed by #1567 or #1993

Comments

@lkeegan
Copy link
Member

lkeegan commented Jul 18, 2019

issue

The parser assumes that a double has a '.' as the decimal separator:

symengine/symengine/parser/parser.ih

Line 100 in f7e253f

if (expr.find_first_of('.') == std::string::npos && lendptr == startptr+expr.length()) {

and if it finds one, it uses strtod to convert the string into a double.

This works fine, unless the locale has been set to one where a ',' is used as the separator instead, then strtod will incorrectly convert e.g. 3.142 to 3, since it uses the locale defined decimal separator:

strtod Parses the C-string str interpreting its content as a floating point number (according to the current locale) and returns its value as a double
http://www.cplusplus.com/reference/cstdlib/strtod/

to reproduce

add the line

    std::setlocale(LC_NUMERIC, "de_DE.UTF-8");

to

symengine/symengine/tests/basic/test_parser.cpp

Line 586 in f7e253f

std::string s;

and the "Parsing: doubles" test will fail.

solution

One solution might be to set the locale to the default 'C' one within the parser:

std::locale old_locale = std::locale::global(std::locale::classic());
// parser code here
std::locale::global(old_locale);

Alternatively just adding a warning that the locale should not be set to something other than the default one somewhere in the docs/wiki would be nice to avoid other users from stumbling across this problem.

Thanks for a great library!
@isuruf
Copy link
Member

isuruf commented Jul 19, 2019

I tried reproducing, and couldn't make the test fail. Is it just the std::setlocale(LC_NUMERIC, "de_DE.UTF-8"); that is needed?

@lkeegan
Copy link
Member Author

lkeegan commented Jul 20, 2019

Maybe you don't have the de_DE.UTF-8 locale on your system? (I think an invalid setlocale call is just silently ignored)

Here is a fork with the extra line:

master...lkeegan:master

and a simple travis build showing the failing test:

https://travis-ci.org/lkeegan/symengine/builds/561374241

@isuruf isuruf linked a pull request Jul 20, 2019 that will close this issue
Open
@lkeegan lkeegan mentioned this issue May 20, 2021
Closed
lkeegan added a commit to lkeegan/symengine that referenced this issue Nov 24, 2023
@lkeegan
Make double parsing locale independent (and faster)
50bc3b1 
- use `std::from_chars` instead of `strtod` ro parse doubles if available (c++17 required)
  - otherwise falls back to previous `strtod` implementation
  - `std::from_chars` is locale independent and faster than `strtod`
- add more double parsing tests
- add double parsing google benchmarks
- resolves symengine#1566
lkeegan added a commit to lkeegan/symengine that referenced this issue Nov 24, 2023
@lkeegan
Make double parsing locale independent (and faster)
a9321e8 
- use `std::from_chars` instead of `strtod` ro parse doubles if available (c++17 required)
  - otherwise falls back to previous `strtod` implementation
  - `std::from_chars` is locale independent and faster than `strtod`
- add more double parsing tests
- add double parsing google benchmarks
- resolves symengine#1566
lkeegan added a commit to lkeegan/symengine that referenced this issue Nov 24, 2023
@lkeegan
Make double parsing locale independent (and faster)
8a388b6 
- use `std::from_chars` instead of `strtod` to parse doubles if available (c++17 required)
  - otherwise falls back to previous `strtod` implementation
  - `std::from_chars` is locale independent and faster than `strtod`
- add more double parsing tests
- add double parsing google benchmarks
- resolves symengine#1566
@lkeegan lkeegan linked a pull request Nov 24, 2023 that will close this issue
Open
lkeegan added a commit to lkeegan/symengine that referenced this issue Feb 20, 2024
@lkeegan
Make double parsing locale independent (and faster)
97a9980 
- use `std::from_chars` instead of `strtod` to parse doubles if available (c++17 required)
  - otherwise falls back to previous `strtod` implementation
  - `std::from_chars` is locale independent and faster than `strtod`
- add more double parsing tests
- add double parsing google benchmarks
- resolves symengine#1566
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@isuruf @lkeegan