Skip to content

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.

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

Merged
merged 1 commit into from
Dec 11, 2020
Merged

Use C locale for floats in HLSLParser #435

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 33 additions & 4 deletions src/libprojectM/Renderer/hlslparser/src/Engine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@

#include <stdio.h> // vsnprintf
#include <string.h> // strcmp, strcasecmp
#include <stdlib.h> // strtod, strtol

#include <stdlib.h> // strtol
#include <locale>
#include <sstream>

namespace M4 {

Expand Down Expand Up @@ -40,7 +41,11 @@ int String_Printf(char * buffer, int size, const char * format, ...) {
}

int String_FormatFloat(char * buffer, int size, float value) {
return String_Printf(buffer, size, "%f", value);
std::ostringstream oss;
oss.imbue(std::locale("C"));
oss << value;

return String_Printf(buffer, size, "%s", oss.str().c_str());
}

bool String_Equal(const char * a, const char * b) {
Expand All @@ -59,8 +64,32 @@ bool String_EqualNoCase(const char * a, const char * b) {
#endif
}

static inline double iss_strtod(const char * in, char ** end) {
Copy link

@afeinberg afeinberg Dec 5, 2020

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

Copy link
Contributor Author

@tatokis tatokis Dec 5, 2020

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;
}

Copy link

@afeinberg afeinberg Dec 5, 2020

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

Copy link
Contributor Author

@tatokis tatokis Dec 5, 2020 •
edited
Loading

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.

char * in_var = const_cast<char *>(in);
double df;
std::istringstream iss(in);
iss.imbue(std::locale("C"));
iss >> df;
if(iss.fail()) {
*end = in_var;
return 0.0;
}
if(iss.eof()) {
*end = in_var + strlen(in);
return df;
}

std::streampos pos = iss.tellg();
if(iss.fail()) {
*end = in_var;
return 0.0;
}
*end = in_var + pos;
return df;
}

double String_ToDouble(const char * str, char ** endptr) {
return strtod(str, endptr);
return iss_strtod(str, endptr);
}

int String_ToInteger(const char * str, char ** endptr) {
Expand Down