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

Rounding in parser doesn't work/is not used #244

Open
Flamefire opened this issue Dec 28, 2024 · 1 comment
Open

Rounding in parser doesn't work/is not used #244

Flamefire opened this issue Dec 28, 2024 · 1 comment

Comments

@Flamefire
Copy link
Contributor

The code to check for rounding isn't tested:

if (offset > significand_buffer_size)
{
offset = significand_buffer_size - 1;
i = significand_buffer_size;
if (significand_buffer[offset] == '5' ||
significand_buffer[offset] == '6' ||
significand_buffer[offset] == '7' ||
significand_buffer[offset] == '8' ||
significand_buffer[offset] == '9')
{
round = true;
}
}

I guess the initial idea was:

This IMO needs an additional overflow check in the rounding (the +=1)

@Flamefire Flamefire changed the title Missing test for round in parser Rounding in parser doesn't work/is not used Dec 29, 2024
@Flamefire
Copy link
Contributor Author

Upon further inspection I changed the title to reflect that this rounding is not used and not only not tested, see https://app.codecov.io/gh/boostorg/charconv/blob/develop/include%2Fboost%2Fcharconv%2Fdetail%2Fparser.hpp#L316

Additionally I think it doesn't even do what it should:

  • The parser interface seems to be used only for (some) long double and by from_chars_float_impl if the format is "hex"
  • for the latter the rounding doesn't make sense as the significant buffer is sized for base 10 so if that is full, with base 16 the value will be too large returning an out-of-range later
  • for the long double case (which is only used on some platforms, e.g. not for MSVC or if long double == double) the rounding case cannot be entered because i <= significand_buffer_size is ensured by the prior code

That might actually introduce a rounding error violating the specification: E.g. "Rounding" the uint64 significant to double (53bit resolution) where remaining decimal digits are ignored instead of used for round can lead to a different rounding for some values as the rounding to the last digit of the significant might cause a carry to the place used for rounding to double,

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

1 participant