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.

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

Set LC_NUMERIC=C to avoid issues reading files #120

Closed
wants to merge 2 commits into from
Closed

Set LC_NUMERIC=C to avoid issues reading files #120

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93309f3
Set LC_NUMERIC=C to avoid issues reading files
ahcorde Nov 4, 2020
1dc83b0
Merge branch 'ign-common3' into ahcorde/set_locale
j-rivero Nov 16, 2020
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
5 changes: 4 additions & 1 deletion graphics/include/ignition/common/MeshLoader.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#ifndef IGNITION_COMMON_MESHLOADER_HH_
#define IGNITION_COMMON_MESHLOADER_HH_

#include <clocale>
#include <string>

#include <ignition/common/graphics/Export.hh>
Expand All @@ -32,7 +33,9 @@ namespace ignition
class IGNITION_COMMON_GRAPHICS_VISIBLE MeshLoader
{
/// \brief Constructor
public: MeshLoader() = default;
public: MeshLoader() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public: MeshLoader() {
public: MeshLoader()
{

std::setlocale(LC_NUMERIC, "C");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in one of the meetings, let's decline this PR and try to address the underlying issue instead.

}

/// \brief Destructor
public: virtual ~MeshLoader() = default;
Expand Down