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

Error in convertFromString<double> #120

Closed
moritzknaust opened this issue Oct 11, 2019 · 4 comments
Closed

Error in convertFromString<double> #120

moritzknaust opened this issue Oct 11, 2019 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@moritzknaust
Copy link

I found a strange error in the convertFromString method. If I use the behaviortree in a rqt-gui plugin, the conversion from string to double is wrong. For example "1.23" is converted to 1 instead of 1.23. This only happens if I use a rqt-gui plugin. When I use the behaviortree in a normal ROS node everything works fine.

I think this error is related to this topic: https://www.reddit.com/r/cpp/comments/2e68nd/stdstod_is_locale_dependant_but_the_docs_does_not/
Maybe Qt changes the used locale to another value, but I am not sure about this.

I have fixed it by using std::istringstream for the conversion instead of std::stod like it is explained in the post. My new version of convertFromString in basic_types.cpp is now the following:

template <>
double convertFromString<double>(StringView str)
{
    std::istringstream iss(str.data());
    iss.imbue(std::locale("C"));
    double temp;
    iss >> temp;
    return temp;
}

This error is very confusing and generates really unexpected behaviors, so can you please fix it? But all in all I really like the library!

@facontidavide
Copy link
Collaborator

facontidavide commented Oct 11, 2019

Honestly, this change makes me a little uncomfortable because of the poor performance of std::istringstream-

In a perfect world, I would like to use this, https://en.cppreference.com/w/cpp/utility/from_chars

But it is c++17 :(

Using this would be a solution https://abseil.io/about/design/charconv
But despite my love for Abseil, I don't want to add this dependency....

@facontidavide facontidavide self-assigned this Oct 11, 2019
@facontidavide facontidavide added the bug Something isn't working label Oct 11, 2019
@moritzknaust
Copy link
Author

I'm not really an expert in C++ development, so I think I cannot help you much at this point. I only found the error and this was the first solution I found...

@facontidavide
Copy link
Collaborator

I will try to figure out a solution. Thanks for reporting the issue

facontidavide added a commit that referenced this issue Nov 21, 2019
@facontidavide
Copy link
Collaborator

I found a solution that is faster. Fixed.

http://quick-bench.com/DWaXRWnxtxvwIMvZy2DxVPEKJnE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants