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

Add generic (template) methods for to_string (float or double). #160

Open
floitsch opened this issue Apr 11, 2021 · 2 comments
Open

Add generic (template) methods for to_string (float or double). #160

floitsch opened this issue Apr 11, 2021 · 2 comments

Comments

@floitsch
Copy link
Collaborator

In #158 (comment) @N-Dekker suggests that we should add a templated ToShortest method that works differently depending on the template argument.

@floitsch
Copy link
Collaborator Author

I don't see the toShortest and StringTo as the same thing.

The ToShortest specifies the size of the output, whereas the templated StringTo gives an information on the input.

It's not completely clear that conversion.ToShortest(float) and conversion.ToShortest(double) should generate different results for the same number. Fundamentally, the size of the output is independent of the input. (Although we currently make it hard to call ToShortestSingle with a double, it would actually work by changing the type from float to double).

That said, I can see the use-case where a program just wants to guarantee round-tripping. Since that shouldn't be too common I would template the function that calls into the double-conversion library (as you probably already did).

As a consequence I'm currently leaning towards not adding these methods.

@N-Dekker
Copy link
Contributor

@floitsch Thank you for opening this issue, Florian. I just tried the existing ToShortest and ToShortestSingle for both double and float:

converter.ToShortest(0.1, &builder1); // double value
std::puts(builder1.Finalize()); // Prints "0.1".

converter.ToShortest(0.1f, &builder2); // float value
std::puts(builder2.Finalize()); // Prints "0.10000000149011612".

converter.ToShortestSingle(0.1f, &builder3); // float value
std::puts(builder3.Finalize()); // Prints "0.1".

Obviously, in this case, ToShortest is not "the shortest" for float 🤔 ToShortestSingle seems preferable for float, while ToShortest should be used for double, generally speaking. Right?

Given the existing member functions, it seems quite likely to me that users will write code like (in pseudo code):

  if type is float do call ToShortestSingle
  else if type is double do call ToShortest

And I found such user code in a couple of places:

https://github.com/kpu/misc/blob/03404f110af56c5e87775111b5870eb227c2f2f0/util/fake_ofstream.hh#L31-L42

And ITK (as I mentioned before at #159 (comment)): https://github.com/InsightSoftwareConsortium/ITK/blob/v5.2.0/Modules/Core/Common/src/itkNumberToString.cxx#L26-L41

As well as duplicate code for float and double, with the float version calling ToShortestSingle and the double version calling ToShortest:
https://github.com/pocoproject/poco/blob/134558f926fadcd446a397f3440ff7f687138ccc/Foundation/src/NumericString.cpp#L112-L122
https://github.com/pocoproject/poco/blob/134558f926fadcd446a397f3440ff7f687138ccc/Foundation/src/NumericString.cpp#L140-L150

So I hope you can consider my pull request #159 "Add DoubleToStringConverter::ToShortestString overload set"

N-Dekker added a commit to N-Dekker/ITK that referenced this issue Apr 13, 2021
Simplified ITK's `FloatingPointNumberToString` implementation by using `DoubleToStringConverter::ToShortestString` overload set, proposed by pull request google/double-conversion#159

Related to issue google/double-conversion#160 "Add generic (template) methods for to_string (float or double)", opened by Florian Loitsch (floitsch).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants