Use locale independent conversion from double to string by simonschmeisser · Pull Request #1099 · moveit/moveit

simonschmeisser · 2018-10-18T14:56:16Z

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 ...)

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

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.

rhaschke · 2018-10-18T20:44:26Z

moveit_planners/ompl/ompl_interface/src/ompl_interface.cpp

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


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

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.

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 · 2018-10-19T08:37:48Z

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

simonschmeisser · 2018-10-19T08:55:17Z

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 · 2018-10-22T13:40:43Z

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

gavanderhoorn · 2018-10-22T13:43:04Z

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

https://status.github.com/messages

rhaschke

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?

moveit_core/utils/include/moveit/utils/lexical_casts.h

rhaschke · 2018-10-22T15:32:11Z

moveit_core/utils/include/moveit/utils/lexical_casts.h

+{
+/** \brief Convert a double to std::string without using the system locale
+
+Depending on the system locale, a different decimal seperator might be used


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

rhaschke · 2018-10-22T15:34:53Z

moveit_core/utils/src/lexical_casts.cpp

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


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

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.

@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.

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

simonschmeisser · 2018-10-23T07:19:18Z

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 · 2018-10-23T09:09:48Z

There is only one more places where the conversion double -> string is done incorrectly:
https://github.com/ros-planning/moveit/blob/5a1d5a8f172c43aedee9f47926068e3ac529d312/moveit_ros/benchmarks/benchmarks/src/benchmark_execution.cpp#L812

should I include it in this PR?

rhaschke · 2018-10-23T15:37:50Z

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

Yes, please.

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

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

moveit_planners/ompl/ompl_interface/src/ompl_interface.cpp

davetcoleman · 2018-10-25T21:31:36Z

moveit_core/CMakeLists.txt

    moveit_distance_field
    moveit_kinematics_metrics
    moveit_dynamics_solver
+    moveit_utils


this is not a package, remove

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

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

davetcoleman · 2018-10-25T21:31:51Z

moveit_core/utils/include/moveit/utils/lexical_casts.h

+/* Author: Simon Schmeisser */
+
+#ifndef MOVEIT_UTILS_
+#define MOVEIT_UTILS_


add MOVEIT_CORE_ to front

davetcoleman · 2018-10-25T21:32:40Z

moveit_core/utils/include/moveit/utils/lexical_casts.h

+#include <string>
+namespace moveit
+{
+namespace utils


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

davetcoleman · 2018-10-25T21:33:04Z

moveit_core/utils/include/moveit/utils/lexical_casts.h

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


Suggested change

std::string toString(float d);

std::string toString(float f);

var name makes more sense

davetcoleman · 2018-10-25T21:36:22Z

moveit_core/utils/src/lexical_casts.cpp

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


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

davetcoleman · 2018-10-25T21:36:50Z

moveit_core/utils/src/lexical_casts.cpp

+  return toStringImpl(d);
+}
+
+template <class T>


I think T would be clearer if called something like OutputDataType

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.

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

rhaschke · 2018-10-26T13:16:41Z

LGTM

davetcoleman · 2018-10-26T17:06:44Z

moveit_core/CMakeLists.txt

    moveit_distance_field
    moveit_kinematics_metrics
    moveit_dynamics_solver
+    moveit_utils


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

* 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 · 2018-10-26T17:12:45Z

Melodic: #1155

* 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

* Remove OgrePrerequisites include * octomap_render.h includes itself * Include OgrePrerequisites.h

simonschmeisser added 2 commits October 18, 2018 16:18

clang format

9f59e1b

rhaschke requested changes Oct 18, 2018

View reviewed changes

simonschmeisser added 2 commits October 22, 2018 14:39

factor out conversion from double/float to string into moveit_utils

cbafac0

clang format

b5ff13e

rhaschke requested changes Oct 22, 2018

View reviewed changes

more explicit description

8151231

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

simonschmeisser added 2 commits October 23, 2018 09:40

adress documentation comments

f379a54

add and use conversion functions from string to double

36a0e9b

simonschmeisser added 3 commits October 24, 2018 09:23

Don't write localized doubles to benchmark results

6f4c37d

use template internally to avoid code duplication

d347e13

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

89b6caf

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 approved these changes Oct 24, 2018

View reviewed changes

moveit_planners/ompl/ompl_interface/src/ompl_interface.cpp Outdated Show resolved Hide resolved

rhaschke added the awaits 2nd review one maintainer approved this request label Oct 24, 2018

remove extra parantheses again

aeeb32e

davetcoleman requested changes Oct 25, 2018

View reviewed changes

Code review comments by Dave

40434d9

simonschmeisser mentioned this pull request Oct 26, 2018

use helper function strToDouble for locale independent parsing of colors ros/urdfdom_headers#47

Merged

davetcoleman approved these changes Oct 26, 2018

View reviewed changes

davetcoleman merged commit e855e7f into moveit:kinetic-devel Oct 26, 2018

rhaschke mentioned this pull request Oct 26, 2018

cherry picking from Kinetic #1160

Merged

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

Use locale independent conversion from double to string (moveit#1099)

2e6f28f

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

Use locale independent conversion from double to string (#1099)

57bfc85

IanTheEngineer mentioned this pull request Oct 30, 2018

A spurious dependency on eigen_msgs is causing OMPL to fail to build. #1174

Merged

7 tasks

JafarAbdi pushed a commit to JafarAbdi/moveit that referenced this pull request Mar 24, 2022

Remove include of OgrePrerequisites header (moveit#1099)

54d2983

* Remove OgrePrerequisites include * octomap_render.h includes itself * Include OgrePrerequisites.h

JafarAbdi mentioned this pull request Mar 31, 2022

[ompl] Small code refactor moveit/moveit2#1138

Merged

6 tasks

v4hn mentioned this pull request Jan 23, 2023

Runtime issues caused by locale-dependent double parsing moveit/moveit2#1882

Open

traversaro mentioned this pull request Mar 14, 2023

URDF Parsers' code is implicitly assuming that "C" locale is set gbionics/idyntree#288

Closed

	std::string toString(float d);
	std::string toString(float f);

Conversation

simonschmeisser commented Oct 18, 2018

Uh oh!

rhaschke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhaschke commented Oct 19, 2018

Uh oh!

simonschmeisser commented Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonschmeisser commented Oct 22, 2018

Uh oh!

gavanderhoorn commented Oct 22, 2018

Uh oh!

rhaschke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonschmeisser commented Oct 23, 2018

Uh oh!

simonschmeisser commented Oct 23, 2018

Uh oh!

rhaschke commented Oct 23, 2018

Uh oh!

rhaschke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonschmeisser Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhaschke commented Oct 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davetcoleman commented Oct 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

simonschmeisser commented Oct 19, 2018 •

edited

Loading

simonschmeisser Oct 26, 2018 •

edited

Loading