Set LC_NUMERIC=C to avoid issues reading files #120
Conversation
github-actions
bot
added
🏢 edifice
Signed-off-by: ahcorde <[email protected]>
ahcorde
force-pushed
the
ahcorde/set_locale
branch
from
418096a to
93309f3
Compare
Codecov Report
@@ Coverage Diff @@
## ign-common3 #120 +/- ##
============================================
Coverage 73.99% 74.00%
============================================
Files 69 69
Lines 9433 9435 +2
============================================
+ Hits 6980 6982 +2
Misses 2453 2453
Continue to review full report at Codecov.
|
| @@ -32,7 +33,9 @@ namespace ignition | |||
| class IGNITION_COMMON_GRAPHICS_VISIBLE MeshLoader | |||
| { | |||
| /// \brief Constructor | |||
| public: MeshLoader() = default; | |||
| public: MeshLoader() { | |||
There was a problem hiding this comment.
| public: MeshLoader() { | |
| public: MeshLoader() | |
| { |
|
@j-rivero to add tests. |
| @@ -32,7 +33,9 @@ namespace ignition | |||
| class IGNITION_COMMON_GRAPHICS_VISIBLE MeshLoader | |||
| { | |||
| /// \brief Constructor | |||
| public: MeshLoader() = default; | |||
| public: MeshLoader() { | |||
| std::setlocale(LC_NUMERIC, "C"); | |||
There was a problem hiding this comment.
This is influencing the global state of the process in a way that is not documented and may not be ideal for a library. Furthermore, other part of the process can then set again the LC_NUMERIC to other values, breaking the functionality of the class after it has been created.
There was a problem hiding this comment.
I think that this is relevant to what you explained here gazebosim/gz-rendering#136, right? So instead of this PR we should try to identify which part of the code is using locale dependent commands and apply changes there.
There was a problem hiding this comment.
Exactly. To be perfectly clear, it is not tragic if this is merged, but I still wanted to raise the point that is not ideal to perturb the global state from a library.
There was a problem hiding this comment.
I agree, this is kind of a concerning workaround. It may not break anything as-is, but it's sort of a bad "code smell" that this is necessary.
There was a problem hiding this comment.
As discussed in one of the meetings, let's decline this PR and try to address the underlying issue instead.
|
We should address the underlying issue instead. |
Related with this issue gazebosim/gz-rendering#101
Signed-off-by: ahcorde [email protected]