This patch fixes bug #3293 - OpenCL kernel argument problem with some locales#3294
Merged
umar456 merged 1 commit intoarrayfire:masterfrom Nov 21, 2022
Merged
Conversation
umar456
force-pushed
the
toString_C_locale
branch
2 times, most recently
from
1dc3a33 to
7ab1bf8
Compare
umar456
previously approved these changes
Nov 16, 2022
umar456
force-pushed
the
toString_C_locale
branch
2 times, most recently
from
0135aa2 to
49e8f03
Compare
The arguments provided to OpenCL uses the C++ standard library function std::to_string(). This function uses the locale to render it's argument to a string. It is a problem when arrayfire is used in a software initialised with non "C" locale. For instance, on a French computer, to_string(1.0) will output the string "1,0000000". This string is provided to OpenCL kernels, generating a syntax error. The most portable way to fix this problem is to use a local ostringstream imbued withe "C" locale. An Other way would be to use C++17 to_chars function, as it only renders it argument with "C" locale, without impact from the application or system locale. The patch is pretty simple, it changes the toString() function to use the stringstream in src/backend/common/TemplateArg.cpp and changed the to_string calls to this toString function in types.cpp.
umar456
force-pushed
the
toString_C_locale
branch
from
49e8f03 to
98022df
Compare
Member
|
@GuillaumeSchmid I made some changes to your PR to address a couple of other issues related to the structure of the codebase. Could you give these changes another try to see if addresses the issue you are seeing. I was able to incorporate the to_chars feature suggested in your comment. |
Contributor
Author
|
@umar456 I tested your implementation with to_char on my code, it works. |
umar456
approved these changes
Nov 21, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Short Description:
There is a problem when the locale is non C when rendering the arguments to OpenCL kernels.
Short description of the change:
The patch is pretty simple, it changes the toString() function to use the stringstream imbued with C locale in src/backend/common/TemplateArg.cpp and changed the to_string calls to this toString function in types.cpp.
Description
The arguments provided to OpenCL uses the C++ standard library function std::to_string(). This function uses the locale to render it's argument to a string.
It is a problem when arrayfire is used in a
software initialised with non "C" locale.
For instance, on a French computer, to_string(1.0) will output the string "1,0000000".
This string is provided to OpenCL kernels, generating a syntax error.
The most portable way to fix this problem is to use a local ostringstream imbued withe "C" locale.
An Other way would be to use C++17 to_chars function, as it only renders it argument with "C" locale, without impact from the application or system locale.
I did not use this latest solution as it would force the use of C++17.