-
-
Notifications
You must be signed in to change notification settings - Fork 418
Use C locale for floats in HLSLParser #435
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
Use C locale for floats in HLSLParser #435
Conversation
| #endif | ||
| } | ||
|
|
||
| static inline double iss_strtod(const char * in, char ** end) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some third party libraries may already do this:
folly:::Conv https://github.com/facebook/folly/blob/master/folly/docs/Conv.md
boost::lexical_cast https://www.boost.org/doc/libs/1_42_0/libs/conversion/lexical_cast.htm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, using either of those would bring in another dependency, which isn't really a good idea for only this specific thing. I'm also not sure if those libraries support getting the end of the float in the string, which is needed for the parser.
I may be wrong though as I am not familiar with either of those, and I only had a quick glance at the links.
Additionally, as mentioned, the rest of the code does the same thing, which is why I think it's the best course of action.
projectm/src/libprojectM/MilkdropPresetFactory/Parser.cpp
Lines 1285 to 1300 in fd1d235
| int Parser::string_to_float(char * string, float * float_ptr) | |
| { | |
| if (*string == 0) | |
| return PROJECTM_PARSE_ERROR; | |
| std::istringstream iss(string); | |
| iss.imbue(std::locale("C")); | |
| iss >> (*float_ptr); | |
| if (!iss.fail()) { | |
| return PROJECTM_SUCCESS; | |
| } | |
| (*float_ptr) = 0; | |
| return PROJECTM_PARSE_ERROR; | |
| } |
| #endif | ||
| } | ||
|
|
||
| static inline double iss_strtod(const char * in, char ** end) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let the compiler decide for itself if it wants to inline this or not: is my advice.
alternativelhy you can use an anonymous namespace instead of static
// what would be a static before
} // anonymous namespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, inline is a suggestion to the compiler. It doesn't force it to do anything. The static was there to make inlining it easier, as when used on a function like this. It makes it accessible only inside that source file/translation unit.
I do agree though. It was only done like that because it's used in that specific place.
If you think it's better, I can make it a normal function that's part of the class, private or public.
2afbbf8 to
52f8764
Compare
|
Good to merge? |
|
Ok, thanks a lot for this improvement! |
The HLSL parser uses locale-aware functions to parse the shaders, which results in errors parsing all shaders under certain locales.
For example, when a decimal point on a float is denoted by a
,instead of a..This results in
This PR does something similar to dfd335f but for HLSLParser.