Partial replace call to snprintf for '%a' and float or double. #7285
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 fetch digger
dub run digger -- build "master + phobos#7285" |
|
As I mentioned above, I'm almost finished with the work on '%e' - it works allready for all floats, but the code needs some refactorisation, I did not check doubles and probably I have to do something for speedup; I think I can do the final tests next weekend. Now I'm not sure if we want to do all four specifiers (%g should be easy once %f and %e works) separate or all in one PR. What do you prefere? |
|
One more thing: %f currently does not pass one test, which is due to a different rounding tie. I meanwhile thought, that it might be easier to first use the old tie (so all tests pass) and then address this in a separate PR. In this case I should change it here too for consistency. What do you think? |
Yes please.
I'm not sure I follow (I have a cold that the moment so its probably that), but do whatever you think is best. |
Oh, my best wishes for a speedy recovery! :-) |
|
I changed everything you requested locally. Before uploading it, I'll rerun the external tests to make sure I didn't break anything. This will take some time. |
|
Ping me when you're good to go. |
|
@thewilsonator All external tests passed. :-) |
FYI: You NEVER need to worry about DMD when doing perf analysis for druntime or phobos. DMD is absolutely slow and years ago it was decided that it's a massive waste of time to bother about it ;-) |
There was a problem hiding this comment.
This pull request introduced a performance regression for compilation time for a "Hello world" program:
https://perf.dlang.io/#program-hello-compile-callgrind-insns;;1575616914.215833;1575724556.863705
| @@ -62,6 +62,7 @@ import std.exception; | |||
| import std.meta; | |||
| import std.range.primitives; | |||
| import std.traits; | |||
| import std.math : FloatingPointControl; | |||
There was a problem hiding this comment.
Perhaps this top-level import is the cause.
Would it be possible to move it to a local import, by splitting the definition of the printFloat function into an eponymous template?
There was a problem hiding this comment.
I'll take care.
Probably I'll use a local enum for rounding modes instead of the solution with eponymous templates. That has the advantage, that I can add the fifth mode that IEEE allows for printing floats, which FloatingPointControl does not know about. (I plan a PR to make formatting floating point numbers pure by using this fifth mode for format and add a separate (nonpure) function, probably formatIEEE for rounding according to the processor state. I will need this enum there anyway.)
Shall I base this PR on stable, as it's some sort of regression?
There was a problem hiding this comment.
Reading the floating point rounding mode is explicitly allowed in pure code:
As a concession to practicality, a pure function can also:
- read and write the floating point exception flags
- read and write the floating point mode flags, as long as those flags are restored to their initial state upon function entry
- perform impure operations in statements that are in a ConditionalStatement controlled by a DebugCondition.
Fix performance regression introduced by PR #7285. merged-on-behalf-of: David Nadlinger <code@klickverbot.at>
This is a followup on #7264. This time it implements %a and %A specifiers. Part of this is identical to #7264 to make this PR independed.
I run essentially the same tests as in #7264.
Concerning correctness: All tests passed. There where two exceptions:
a) There is a bug for denormalized floats in snprintf. The new version fixes this.
b) Ties for roundToNearest are different, like in #7264.
Concerning speed: dmd is a little bit slower with the new version, while ldc2 is slightly faster. See pdf-file for some diagrams.