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

Partial replace call to snprintf for formating floatingpoint numbers for %f and %F #7757

Merged
merged 11 commits into from
Feb 15, 2021

Conversation

berni44
Copy link
Contributor

@berni44 berni44 commented Jan 20, 2021

This is a copy of #7264 and a followup on #7318 and #7285.

I rebased it, added some minor fixes and a test that can be run to compare the result of snprintf and the result of printFloat.

To run the test uncomment the line // version = printFloatTest; near the end of the file and run a normal unittest. This takes 30 minutes and compares random input. Some known bugs of snprintf are written at the top of the test section. They will, of course, be reported as differences.

For more information see the original PR. I was able to restore the PDF document mentioned there, thanks to @skoppe and @m3m0ry who saved it.

@thewilsonator asked there one question I didn't answer yet:

Following the success of the float zero factorisation, I'm beginning to think that the best way to organise the printing is not by format specifier (%a,%f,%g,%e) but by "category" (for want of a better term), i.e.

  • inf/nan (handled above),
  • Algorithm A: For large exponents (exp >= T.mant_dig)
  • Algorithm B: For small exponents (exp < T.mant_dig - 61)
  • Algorithm C: For exponents close to 0. (already done)

Since there seems to be much more in common between the different ranges of numbers than there is between the different format specifiers.

Yes, you're right. I allready thought about this too (and snprintf does this too). But I'm a little bit reluctant do so immediately for two reasons: First, I fear, that the result would be slower because several if switches might be needed. Second and more important, I have some ideas how cases A and B can be speed up. Implementing these ideas might be much more difficult, when I have to code for all specifiers simultaneously.

@berni44 berni44 requested a review from andralex as a code owner January 20, 2021 17:41
@dlang-bot
Copy link
Contributor

dlang-bot commented Jan 20, 2021

Thanks for your pull request and interest in making D better, @berni44! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
9889 normal Incorrect rounding on floating value formatting
20320 normal format("%f") leeds to wrong output
20363 normal painting XMM registers as integers leads to codegen bugs
20371 normal std.format limited to 500 characters for floats

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7757"

@berni44
Copy link
Contributor Author

berni44 commented Jan 20, 2021

I don't know why the bot thinks issue 20636 is touched here. This is no enhancement!

@RazvanN7
Copy link
Collaborator

Commit message of 686c5e5 contains the following text:

"Bug 20636 has meanwhile been fixed, so no hack needed anymore
Remove two missleading comments (there is no more cast needed)
Adapt new convention for naming issues
Add missing spaces arround operators"

The bolded words have triggered the bot. Maybe you can rephrase the commit message to avoid those words.

@berni44 berni44 force-pushed the printFloatF branch 2 times, most recently from 823f992 to 66290b9 Compare January 21, 2021 12:47
@berni44
Copy link
Contributor Author

berni44 commented Jan 21, 2021

The number was wrong anyway: 20636 instead of the correct 20363... Now, at least, he added the bug fix label, but the enhancement remains... Maybe, someone with write access could just delete the enhancement label?

@schveiguy
Copy link
Member

I mean, it is an enhancement, no? Or does this not remove the requirement of snprintf?

@berni44
Copy link
Contributor Author

berni44 commented Jan 22, 2021

I mean, it is an enhancement, no? Or does this not remove the requirement of snprintf?

Well, it replaces snprintf (partially). But is that already an enhancement? Is it an enhancement replacing quicksort by timsort internally? I don't know. Might depend on the definition of enhancement...

@PetarKirov
Copy link
Member

Making Druntime and Phobos more independent of libc (even partially) is an important improvement in my book.

Copy link
Collaborator

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

Approving the idea, however, this needs a proper review.

// digits. Finally, we decide the rounding type, mainly by looking at the next digit.

ulong[18] bigbuf;
if (exp >= T.mant_dig)
Copy link
Member

Choose a reason for hiding this comment

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

You mention "does not fit into a ulong" below. Are you assuming that mant_dig is always <= 64? What happens when the mant_dig is 106 or 113?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR targets only floats, doubles and 64bit reals. See the template constraints of this function.
The "does not fit" comment refers to the unrolled number, where all the zeros (counted in exp) are attached either left or right to the mantissa; these are really large numbers, taking up to a little bit more than 1000 bits.

@berni44
Copy link
Contributor Author

berni44 commented Jan 27, 2021

I wrote some explanations accompanied by source code snippets and illustrations:
https://github.com/berni44/printFloat/blob/master/walkthrough.pdf
I hope, this will make the review easier and tear down the fears...

@berni44
Copy link
Contributor Author

berni44 commented Jan 28, 2021

Resolved merge conflicts.

With PR #7762 being merged, printing floats with %f will automatically be CTFEable too. I added unittests and corrected the error message.

I also fixed a small bug in an other unittest - FloatingPointControl is not available on targets with soft float.

std/format.d Outdated
if (mybig[lsu] == 0)
++lsu;

buffer[right++] = cast(byte) ('0'+over);
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR has a number of inconsistent (missing) whitespace between binary operands, but otherwise looks fine.

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

See comments

berni44 and others added 8 commits February 15, 2021 11:11
Bug 20363 (which has acidentially been written as 20636) has meanwhile
  been taken care of, so no hack needed anymore
Remove two missleading comments (there is no more cast needed)
Adapt new convention for naming bug reports
Add missing spaces arround operators
@thewilsonator thewilsonator merged commit fc79d0a into dlang:master Feb 15, 2021
@jondegenhardt
Copy link
Contributor

jondegenhardt commented Mar 14, 2021

This PR ended up fixing a case of incorrect floating point rounding occurring on Windows in the tsv-utils test suite. 👍

@berni44 berni44 deleted the printFloatF branch April 28, 2021 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants