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

Conversion from double to string is locale-dependent if TOML_FLOATING_POINT_CHARCONV is 0 #19

Closed
traversaro opened this issue Apr 5, 2020 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@traversaro
Copy link

traversaro commented Apr 5, 2020

Environment

Compiler:
Any compiler that does not support to_chars for double , i.e. for which TOML_FLOATING_POINT_CHARCONV is 0.

C++ standard mode (e.g. 17, 20, 'latest'):
17

Target arch (e.g. x64):
Not relevant.

Exceptions enabled:
Not relevant.

Relevant toml++ configuration customizations:
Not relevant.

Relevant compilation flags:
Not relevant.

Describe the bug

When TOML_FLOATING_POINT_CHARCONV std::ostringstream is used to convert a double to a string in https://github.com/marzer/tomlplusplus/blob/v1.1.0/include/toml++/toml_parser_impl.h#L153 . However, the imbue method is not called, so the default locale, that is the global C++ locale set in https://en.cppreference.com/w/cpp/locale/locale/global . If it is intended to have the same behavior independently from the value TOML_FLOATING_POINT_CHARCONV, I guess it is necessary to call ss.imbue(std::locale::classic()); after the ss object is declared. I did not checked other code paths if TOML_FLOATING_POINT_CHARCONV is set to 0, but it is possible that they are affected as well.

Additional information

Related issues:

cc @S-Dafarra @GiulioRomualdi

@traversaro traversaro added the bug Something isn't working label Apr 5, 2020
@marzer
Copy link
Owner

marzer commented Apr 5, 2020

Oh, thanks for bringing this to my attention @traversaro!

@marzer
Copy link
Owner

marzer commented Apr 6, 2020

@traversaro I've implemented a fix locally but in testing some additional locale machinery I uncovered a few other related issues, which I'll fix tomorrow before pushing anything.

@marzer marzer closed this as completed in 5ca6b29 Apr 7, 2020
@marzer
Copy link
Owner

marzer commented Apr 7, 2020

@traversaro The fix for this is live now. Solving this forced me to learn about locales on unix and to add different locales to my CI pipeline, so any locale-based regressions should be caught in future.

@traversaro
Copy link
Author

Thanks a lot. I reported these kind of locale-related bugs to several open source projects over the years, but I never saw something as complete as the testing infrastructure that you set up!

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