-
-
Notifications
You must be signed in to change notification settings - Fork 717
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
Conversation
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 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
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#7757" |
I don't know why the bot thinks issue 20636 is touched here. This is no enhancement! |
Commit message of 686c5e5 contains the following text: "Bug 20636 has meanwhile been fixed, so no hack needed anymore The bolded words have triggered the bot. Maybe you can rephrase the commit message to avoid those words. |
823f992
to
66290b9
Compare
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? |
I mean, it is an enhancement, no? Or does this not remove the requirement of snprintf? |
Well, it replaces |
Making Druntime and Phobos more independent of libc (even partially) is an important improvement in my book. |
There was a problem hiding this 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I wrote some explanations accompanied by source code snippets and illustrations: |
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments
Currently, only float and double are supported.
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
This PR ended up fixing a case of incorrect floating point rounding occurring on Windows in the tsv-utils test suite. 👍 |
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 ofsnprintf
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:
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 severalif
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.