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

<charconv>: from_chars, incorrect conversion of tiny doubles, when specified without a fractional part #3161

Closed
codingatc opened this issue Oct 18, 2022 · 5 comments · Fixed by #3670
Assignees
Labels
bug Something isn't working fixed Something works now, yay!

Comments

@codingatc
Copy link

I have found a large number of values that do not convert correctly in MSVC's implementation. I compared these inputs to other implementations such as strtod in GLIBC and David Gay's strtod. These other implementations correctly parse in these cases. Here is an example of a case MSVC's from_chars fails:

28017185671564702625986967801367508381305145856029502167789836829722124560807078866183829589255835985468748869481865831423625171646345202433605015840157138072331248713371985585677588002359765561114886055788588809325542179658630705136096846206365989189946205424753774597659687276771418085294584448440805339915561387108213955014557979970724102739077746853707044081281775302910700845395388393628022004086658191413817026937499382994523889310476633341355125369701615122013368655379069904054709590878154706273368602670826427058187248909234850990764074278131969928218903968141467357461486306298730685574629176781197826437147132104622720085189484313941668795347347888407545459216605573014774895683738184268219531683594714240155669493243656420489173797250259667634963989257812500000001e-1083

This value is only slightly larger than an exact round point. However the way I have constructed the number confuses the code in charconv. Notably, it processes the entire number in the "Scan the integer part of the mantissa" phase of the code. This part of the code never sets _Has_zero_tail. Then the code breaks out of the "Scan the fractional part of the mantissa" loop on the first iteration due to, if (_Digit_value >= _Base), never setting _Has_zero_tail.

My naive fix of adding the else _Has_zero_tail = _Has_zero_tail && _Digit_value == 0; to the integer part as well, seems to work.

@codingatc
Copy link
Author

In brief the code discards the trailing 1, and rounds down, rather than up.

@CaseyCarter CaseyCarter added the bug Something isn't working label Oct 19, 2022
@StephanTLavavej StephanTLavavej self-assigned this Oct 19, 2022
@StephanTLavavej
Copy link
Member

Thanks for the bug report! I'll investigate (this might be similar to issues we've previously fixed, but I'm not sure yet).

@codingatc
Copy link
Author

Stephan, this is different than the bug I previously reported, and you fixed. I checked the code on top of tree, and it appears to still be incorrect there. However, I didn't test TOT either, because I am not sure how to point Visual Studio to use the code here, instead of the installed version.

@frederick-vs-ja
Copy link
Contributor

This looks like #3375 to me. I guess the method suggested by @statementreply (#3375 (comment)) works.

When the radix of the number system is even and >= 4, to round a number for rounding again to less significant digits later, possibly with different rounding modes, one of the possible algorithms is to add one if and only if sticky && (last_digit == 0 || last_digit == radix/2) (no carrying). For decimal numbers, this means increasing 0 to 1 and 5 to 6, but leaving [12346789] unchanged when discarding trailing digits.

num_get::do_get is rounding the decimal or hexadecimal number for conversion to binary. While it's not exactly the same problem, given the radix (10->2 and 16->2) and precision involved, the algorithm above should also work.

@statementreply
Copy link
Contributor

This looks like #3375 to me. I guess the method suggested by @statementreply (#3375 (comment)) works.

When the radix of the number system is even and >= 4, to round a number for rounding again to less significant digits later, possibly with different rounding modes, one of the possible algorithms is to add one if and only if sticky && (last_digit == 0 || last_digit == radix/2) (no carrying). For decimal numbers, this means increasing 0 to 1 and 5 to 6, but leaving [12346789] unchanged when discarding trailing digits.
num_get::do_get is rounding the decimal or hexadecimal number for conversion to binary. While it's not exactly the same problem, given the radix (10->2 and 16->2) and precision involved, the algorithm above should also work.

I believe that this is unnecessary. num_get::do_get unlocalizes the string and passes it to strto[f|d] for the actual parsing work, which is unaware of dropped digits, so we modify the string such that the result is the same as if the full string were passed to strto[f|d]. from_chars, however, could deal with them directly (_Has_zero_tail).

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

Successfully merging a pull request may close this issue.

5 participants