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.

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 locale independent conversion from double to string #1099

Merged
merged 13 commits into from Oct 26, 2018

Conversation

Projects
None yet
4 participants
@simonschmeisser
Copy link
Contributor

simonschmeisser commented Oct 18, 2018

boost::lexical_caststd::string(0.1) returns "0,1" on Dutch, German and probably some other locales
which boost::lexical_cast fails to parse even on Dutch, German etc locales

so this PR uses std::ostringstream which seems to be the only sane way to ignore locale

(this is no new problem, it somehow seemed to "sometimes" work to put strings (ie '0.1') instead off the raw numbers (ie 0.1) in ompl_planning.yaml but not always ...)

simonschmeisser added some commits Oct 18, 2018

Use locale independent conversion from double to string
boost::lexical_cast<std::string>(0.1) returns "0,1" on Dutch, German and probably some other locales
which boost::lexical_cast<double> fails to parse even on Dutch, German etc locales

so this commit uses std::ostringstream which seems to be the only sane way to ignore locale
@rhaschke
Copy link
Contributor

rhaschke left a comment

I agree to the need of this fix. However, instead of repeating the same code snippet over and over, we should aim for a generic solution at a central place. We already have macros and exceptions. However, both folders have only a single header / source file. I suggest a more generic utils folder:

  • utils/CMakeLists.txt (building libmoveit_utils)
  • utils/include/moveit/utils/conversions.h
  • utils/src/conversions.cpp

There we could implement a moveit::lexical_cast.

{
// convert to string using no locale
std::ostringstream oss;
oss.imbue(std::locale());

This comment has been minimized.

@rhaschke

rhaschke Oct 18, 2018

Contributor

I'm not sure that's really neccessary at all.
If so, I guess you need to use std::locale::classic()

This comment has been minimized.

@simonschmeisser

simonschmeisser Oct 19, 2018

Author Contributor

Yes, using std::locale::classic() is indeed better. Omitting it would probably/usually/maybe also be right unless somebody somewhere set the global locale to something else.

As a side note, using std::stod for the inverse operation does not work as this uses the c locale internally which does seem to get set automatically from the system.

use std::locale::classic() instead of std::locale()
std::locale() copies the global locale, which is already used at construction time of std::stringstream. So this was a noop.
The global locale defaults to std::locale::classic() but it might have been set somewhere else before,
 so explicitly setting std::locale::classic() makes this code independent of locale settings elsewhere.
@rhaschke

This comment has been minimized.

Copy link
Contributor

rhaschke commented Oct 19, 2018

@simonschmeisser Please factor out this conversion code into its own library.

@simonschmeisser

This comment has been minimized.

Copy link
Contributor Author

simonschmeisser commented Oct 19, 2018

sure, @rhaschke , one thing at its time ;) would you prefer

  • a full featured moveit_tools::lexical_cast (that uses template specialization for double/float and boost::lexical_cast otherwise)
  • or just a simple moveit_tools::toStringLocaleIndependent(float/double) and use std::to_string otherwise? (would require a moveit_tools::toString(bool) as well)
@simonschmeisser

This comment has been minimized.

Copy link
Contributor Author

simonschmeisser commented Oct 22, 2018

github sometimes forgets about my further commits, I'm confused

@gavanderhoorn

This comment has been minimized.

Copy link
Member

gavanderhoorn commented Oct 22, 2018

github sometimes forgets about my further commits, I'm confused

https://status.github.com/messages

@rhaschke
Copy link
Contributor

rhaschke left a comment

Thanks for adapting this. Some minor comments inline.

  • Did you checked for all occurences of boost::lexical_cast in the source? Are there any other locations that need an updated?
  • What about conversion fromString to a value? Shouldn't we have appropriate C-locale functions for that too?
Show resolved Hide resolved moveit_core/utils/include/moveit/utils/lexical_casts.h Outdated
{
/** \brief Convert a double to std::string without using the system locale
Depending on the system locale, a different decimal seperator might be used

This comment has been minimized.

@rhaschke

rhaschke Oct 22, 2018

Contributor

I suggest to move this comment to the top as a general comment for this file.

{
namespace utils
{
std::string toString(double d)

This comment has been minimized.

@rhaschke

rhaschke Oct 22, 2018

Contributor

The implementation using function overloading is somewhat redundant.
Why didn't you choose templates as discussed?

This comment has been minimized.

@simonschmeisser

simonschmeisser Oct 23, 2018

Author Contributor

There are only two variable types (float and double) where these functions are required. For other variable types (integer types) std::to_string would be a more obvious solution? This can of course be handled using template specialization if requested but imho just adds compile time and unneeded abstractions.

This comment has been minimized.

@rhaschke

rhaschke Oct 23, 2018

Contributor

@simonschmeisser I always try to prevent code duplication as much as possible - even in this simple case. In this particular case, I would suggest to use a template defined in .cpp only, providing the common implementation for both overloaded functions. This doesn't add compile time or any other complexity to the API.

Please, use github's Reply button to answer such remarks. This way, the thread is kept together.

more explicit description
Co-Authored-By: simonschmeisser <s.schmeisser@gmx.net>
@simonschmeisser

This comment has been minimized.

Copy link
Contributor Author

simonschmeisser commented Oct 23, 2018

about string->float/double: from a bug fixing point of view it's not necessary since boost::lexical_cast(string) expects "." independent of locale (there is not a single line about locales in the documentation of boost::lexical_cast so ...)

from a maintenance point of view (as boost::lexical_cast people do not seem to know what a locale is ...) it might be better to indeed add this inverse function

@simonschmeisser

This comment has been minimized.

Copy link
Contributor Author

simonschmeisser commented Oct 23, 2018

There is only one more places where the conversion double -> string is done incorrectly:

rundata["total_time REAL"] = boost::lexical_cast<std::string>(total_time);

should I include it in this PR?

@rhaschke

This comment has been minimized.

Copy link
Contributor

rhaschke commented Oct 23, 2018

There is only one more place where the conversion double -> string is done incorrectly. Should I include it in this PR?

Yes, please.

simonschmeisser added some commits Oct 24, 2018

Add error handling for std::string->float/double
now throws a std::runtime_exception if errors occur.
For planners this gets caught in planning_pipeline::PlanningPipeline::generatePlan
and there are no further users of toDouble/toFloat currently
@rhaschke
Copy link
Contributor

rhaschke left a comment

Thanks a lot for those fixes. One more nit-pick.

@@ -114,6 +115,7 @@ catkin_package(
moveit_distance_field
moveit_kinematics_metrics
moveit_dynamics_solver
moveit_utils

This comment has been minimized.

@davetcoleman

davetcoleman Oct 25, 2018

Member

this is not a package, remove

This comment has been minimized.

@simonschmeisser

simonschmeisser Oct 26, 2018

Author Contributor

this section is about what libraries are exported by the moveit_core package, you get linker errors in dependent packages if moveit_utils is not added here

This comment has been minimized.

@davetcoleman

davetcoleman Oct 26, 2018

Member

oops sorry. the github code folding hid the relevant section and i made a wrong assumption

/* Author: Simon Schmeisser */

#ifndef MOVEIT_UTILS_
#define MOVEIT_UTILS_

This comment has been minimized.

@davetcoleman

davetcoleman Oct 25, 2018

Member

add MOVEIT_CORE_ to front

#include <string>
namespace moveit
{
namespace utils

This comment has been minimized.

@davetcoleman

davetcoleman Oct 25, 2018

Member

this should have namespace core also, or instead of utils, as used in all the other subfolders of moveit_core

std::string toString(double d);

/** \brief Convert a float to std::string using the classic C locale */
std::string toString(float d);

This comment has been minimized.

@davetcoleman

davetcoleman Oct 25, 2018

Member
Suggested change
std::string toString(float d);
std::string toString(float f);

var name makes more sense

// convert from string using no locale
std::istringstream iss(s);
iss.imbue(std::locale::classic());
T d;

This comment has been minimized.

@davetcoleman

davetcoleman Oct 25, 2018

Member

there are too many one character variable names here, hard to read. add longer variable names

return toStringImpl(d);
}

template <class T>

This comment has been minimized.

@davetcoleman

davetcoleman Oct 25, 2018

Member

I think T would be clearer if called something like OutputDataType

This comment has been minimized.

@rhaschke

rhaschke Oct 25, 2018

Contributor

This is nit-picking. T is the standard for a template type. Only if more than one type is involved, there is a need in more verbose type names.

This comment has been minimized.

@davetcoleman

davetcoleman Oct 26, 2018

Member

fair. just pulling in recent experiences in google-style projects, but ignore my comment

@rhaschke

This comment has been minimized.

Copy link
Contributor

rhaschke commented Oct 26, 2018

LGTM

@@ -114,6 +115,7 @@ catkin_package(
moveit_distance_field
moveit_kinematics_metrics
moveit_dynamics_solver
moveit_utils

This comment has been minimized.

@davetcoleman

davetcoleman Oct 26, 2018

Member

oops sorry. the github code folding hid the relevant section and i made a wrong assumption

@davetcoleman davetcoleman merged commit e855e7f into ros-planning:kinetic-devel Oct 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

davetcoleman added a commit to davetcoleman/moveit that referenced this pull request Oct 26, 2018

Use locale independent conversion from double to string (ros-planning…
…#1099)

* Use locale independent conversion from double to string

boost::lexical_cast<std::string>(0.1) returns "0,1" on Dutch, German and probably some other locales
which boost::lexical_cast<double> fails to parse even on Dutch, German etc locales

so this commit uses std::ostringstream which seems to be the only sane way to ignore locale

* clang format

* use std::locale::classic() instead of std::locale()

std::locale() copies the global locale, which is already used at construction time of std::stringstream. So this was a noop.
The global locale defaults to std::locale::classic() but it might have been set somewhere else before,
 so explicitly setting std::locale::classic() makes this code independent of locale settings elsewhere.

* factor out conversion from double/float to string into moveit_utils

* clang format

* more explicit description

Co-Authored-By: simonschmeisser <s.schmeisser@gmx.net>

* adress documentation comments

* add and use conversion functions from string to double

* Don't write localized doubles to benchmark results

* use template internally to avoid code duplication

* Add error handling for std::string->float/double

now throws a std::runtime_exception if errors occur.
For planners this gets caught in planning_pipeline::PlanningPipeline::generatePlan
and there are no further users of toDouble/toFloat currently

* remove extra parantheses again

* Code review comments by Dave
@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Oct 26, 2018

Melodic: #1155

rhaschke added a commit to ubi-agni/moveit that referenced this pull request Oct 26, 2018

Use locale independent conversion from double to string (ros-planning…
…#1099)

* Use locale independent conversion from double to string

boost::lexical_cast<std::string>(0.1) returns "0,1" on Dutch, German and probably some other locales
which boost::lexical_cast<double> fails to parse even on Dutch, German etc locales

so this commit uses std::ostringstream which seems to be the only sane way to ignore locale

* clang format

* use std::locale::classic() instead of std::locale()

std::locale() copies the global locale, which is already used at construction time of std::stringstream. So this was a noop.
The global locale defaults to std::locale::classic() but it might have been set somewhere else before,
 so explicitly setting std::locale::classic() makes this code independent of locale settings elsewhere.

* factor out conversion from double/float to string into moveit_utils

* clang format

* more explicit description

Co-Authored-By: simonschmeisser <s.schmeisser@gmx.net>

* adress documentation comments

* add and use conversion functions from string to double

* Don't write localized doubles to benchmark results

* use template internally to avoid code duplication

* Add error handling for std::string->float/double

now throws a std::runtime_exception if errors occur.
For planners this gets caught in planning_pipeline::PlanningPipeline::generatePlan
and there are no further users of toDouble/toFloat currently

* remove extra parantheses again

* Code review comments by Dave

rhaschke added a commit to ubi-agni/moveit that referenced this pull request Oct 26, 2018

rhaschke added a commit that referenced this pull request Oct 26, 2018

ggupta9777 added a commit to ggupta9777/moveit that referenced this pull request Mar 17, 2019

Use locale independent conversion from double to string (ros-planning…
…#1099)

* Use locale independent conversion from double to string

boost::lexical_cast<std::string>(0.1) returns "0,1" on Dutch, German and probably some other locales
which boost::lexical_cast<double> fails to parse even on Dutch, German etc locales

so this commit uses std::ostringstream which seems to be the only sane way to ignore locale

* clang format

* use std::locale::classic() instead of std::locale()

std::locale() copies the global locale, which is already used at construction time of std::stringstream. So this was a noop.
The global locale defaults to std::locale::classic() but it might have been set somewhere else before,
 so explicitly setting std::locale::classic() makes this code independent of locale settings elsewhere.

* factor out conversion from double/float to string into moveit_utils

* clang format

* more explicit description

Co-Authored-By: simonschmeisser <s.schmeisser@gmx.net>

* adress documentation comments

* add and use conversion functions from string to double

* Don't write localized doubles to benchmark results

* use template internally to avoid code duplication

* Add error handling for std::string->float/double

now throws a std::runtime_exception if errors occur.
For planners this gets caught in planning_pipeline::PlanningPipeline::generatePlan
and there are no further users of toDouble/toFloat currently

* remove extra parantheses again

* Code review comments by Dave

ipa-hsd added a commit to ipa-hsd/moveit that referenced this pull request Mar 18, 2019

Use locale independent conversion from double to string (ros-planning…
…#1099)

* Use locale independent conversion from double to string

boost::lexical_cast<std::string>(0.1) returns "0,1" on Dutch, German and probably some other locales
which boost::lexical_cast<double> fails to parse even on Dutch, German etc locales

so this commit uses std::ostringstream which seems to be the only sane way to ignore locale

* clang format

* use std::locale::classic() instead of std::locale()

std::locale() copies the global locale, which is already used at construction time of std::stringstream. So this was a noop.
The global locale defaults to std::locale::classic() but it might have been set somewhere else before,
 so explicitly setting std::locale::classic() makes this code independent of locale settings elsewhere.

* factor out conversion from double/float to string into moveit_utils

* clang format

* more explicit description

Co-Authored-By: simonschmeisser <s.schmeisser@gmx.net>

* adress documentation comments

* add and use conversion functions from string to double

* Don't write localized doubles to benchmark results

* use template internally to avoid code duplication

* Add error handling for std::string->float/double

now throws a std::runtime_exception if errors occur.
For planners this gets caught in planning_pipeline::PlanningPipeline::generatePlan
and there are no further users of toDouble/toFloat currently

* remove extra parantheses again

* Code review comments by Dave
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.