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

ENH: Support calling a const NumberToString, add unit tests + style improvements #2256

Conversation

N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented Jan 17, 2021

Supported calling a const NumberToString function object, by declaring all NumberToString::operator() overloads const.

Added various GoogleTest unit tests, which do call const NumberToString objects.

Did various style improvements, including a reduction of the buf size, and removal of builder.Reset() calls from the implementation of NumberToString.

Supported calling a `const` NumberToString function object, by declaring
all `NumberToString::operator()` overloads `const`.

Added various GoogleTest unit tests, which do call `const`
NumberToString objects.
Various style improvements of the implementation of the
`NumberToString<TValue>` specializations for floating point numbers:

 - Did clean up the #include's.
 - Removed unnecessary builder.Reset() calls. (`StringBuilder::Reset()` only just sets the internal position to zero, but that is already taken care of by the constructor of `StringBuilder`.)
 - Added local helper function, `FloatingPointNumberToString(val)`, sharing code between `float` and `double` specialization.
 - Reduced size of local buffer (`buf`) from 256 to 32 chars.
@N-Dekker N-Dekker marked this pull request as ready for review January 17, 2021 20:16
@N-Dekker N-Dekker changed the title ENH: Support calling a const NumberToString, add GoogleTest unit tests ENH: Support calling a const NumberToString, add unit tests + style improvements Jan 17, 2021
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

Great to see the improved coverage 💚

@N-Dekker
Copy link
Contributor Author

N-Dekker commented Jan 18, 2021

FYI, some interesting differences between the output of std::ostringstream and itk::NumberToString<double> that I encountered while writing the unit tests (using NL = std::numeric_limits<double>):

input stream.str() itk::NumberToString
-0.0 -0 0
1e-6 1e-06 0.000001
1e20 1e+20 100000000000000000000
NL::lowest() -1.79769e+308 -1.7976931348623157e+308
NL::epsilon() 2.22045e-16 2.220446049250313e-16
NL::denorm_min() 4.94066e-324 5e-324
NL::min() 2.22507e-308 2.2250738585072014e-308
NL::quiet_NaN() nan NaN
NL::infinity() inf Infinity

As produced by the following code:

for (const double d :
     { -0.0, 1e-6, 1e20, NL::lowest(), NL::epsilon(), NL::denorm_min(), NL::min(), NL::quiet_NaN(), NL::infinity() })
{
  std::ostringstream stream;
  stream << d;
  std::cout << " | " << stream.str() << " | " << itk::NumberToString<double>{}(d) << '\n';
}

@dzenanz
Copy link
Member

dzenanz commented Jan 18, 2021

Now I see your comment. Perhaps in the case of

input stream.str() itk::NumberToString
NL::denorm_min() 4.94066e-324 5e-324

itk::NumberToString is less precise because of a "short" buffer?

@dzenanz dzenanz merged commit c00531f into InsightSoftwareConsortium:master Jan 18, 2021
@dzenanz
Copy link
Member

dzenanz commented Jan 18, 2021

If you wish to add some more unit tests, you can do so in a new PR specifically aimed at increasing test coverage. But that does not seem necessary.

@N-Dekker
Copy link
Contributor Author

If you wish to add some more unit tests, you can do so in a new PR specifically aimed at increasing test coverage. But that does not seem necessary.

@dzenanz Thanks, I think it isn't necessary either to add more unit tests. 😃 I did add them especially to ensure that the STYLE commit c00531f wouldn't change the existing behavior. And that should be fine, as the tests are passed now both before and after the STYLE commit.

BTW, the tests might eventually break if the floating point format of itk::NumberToString is ever going to be changed. Instead of calling EcmaScriptConverter(), itk::NumberToString might be changed to use a different DoubleToStringConverter object, having different formatting flags. https://github.com/google/double-conversion/blob/master/double-conversion/double-to-string.h#L113-L121 But that's not my proposal, for now...!

@N-Dekker
Copy link
Contributor Author

FYI, I just submitted a related issue: "Maximum string length DoubleToStringConverter::EcmaScriptConverter() ToShortest? 26 chars?", google/double-conversion#146

@N-Dekker
Copy link
Contributor Author

Last weekend I submitted a related pull request to Google's double-conversion library, which aims to allow further simplifying the implementation of itk::NumberToString (as well as similar user code). Please have a look:

google/double-conversion#159 "Add DoubleToStringConverter::ToShortestString overload set"

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

Successfully merging this pull request may close these issues.

3 participants