Repository navigation
flt2dec front of format_short & format_fixed - #154505
pascaldekloe wants to merge 6 commits into
Conversation
|
Some changes occurred in float parsing cc @tgross35 |
|
r? @scottmcm rustbot has assigned @scottmcm. Use Why was this reviewer chosen?The reviewer was selected based on:
|
There was a problem hiding this comment.
Thank you for preparing this!
I only had time to go through the first commit here, which mostly looks pretty good to me with a few small requests. If you'd like to fix those then put only the first commit in a separate PR, I'd be happy to approve that bit and shrink the remaining todo here.
As a noncritical note, I'm having a bit of trouble understanding the commit summaries. The messages usually have details, but I'm not entirely sure what something like "write out of flt2dec/random.rs" or "test flt2dec front of format_short & format_fixed" means. We don't really have a guideline but sticking to the usual style with changes described in imperative mood would help a bit (e.g. something like flt2dec: test: Don't use `write!` in random tests or flt2dec: test: Test `format_{short,fixed}` via the main entrypoint).
I'll review the rest a little later.
|
Pushed the commit explainer plus distinct summary lines in Rust docs @tgross35. :-) I'll wait for the rest of the review. Some urgency would be nice because I have time to do this now. 🙏 The git messages ain't readable sections on their own indeed. You need the code in conjunction to comprehend the listings. I'll work on that. |
20351fd to
aaf4362
Compare
aaf4362 to
894fa1e
Compare
|
Let's make sure there's no perf effect. @bors try @rust-timer queue @rustbot author |
This comment has been minimized.
This comment has been minimized.
|
Reminder, once the PR becomes ready for a review, use |
This comment has been minimized.
This comment has been minimized.
flt2dec front of format_short & format_fixed
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (cb7a7f2): comparison URL. Overall result: no relevant changes - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (secondary 3.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 488.817s -> 487.773s (-0.21%) |
This comment has been minimized.
This comment has been minimized.
flt2dec front of format_short & format_fixed The fallback to Dragon was placed in the Grisu module. As a result tests don't test what they appear to test, and some benchmarks from Grisu measured Dragon instead. The corrected values in the tests haven been verified against the Go implementation.
flt2dec front of format_short & format_fixed The fallback to Dragon was placed in the Grisu module. As a result tests don't test what they appear to test, and some benchmarks from Grisu measured Dragon instead. The corrected values in the tests haven been verified against the Go implementation.
|
Enclosed in rollup, |
|
Auto build was cancelled. Cancelled workflows: The next pull request likely to be tested is #164554. |
This comment has been minimized.
This comment has been minimized.
flt2dec front of format_short & format_fixed The fallback to Dragon was placed in the Grisu module. As a result tests don't test what they appear to test, and some benchmarks from Grisu measured Dragon instead. The corrected values in the tests haven been verified against the Go implementation.
|
This pull request was unapproved. This PR was contained in a rollup (#164554), which was unapproved. Auto build was cancelled due to unapproval. Cancelled workflows: |
|
@bors try jobs=test-x86_64-mingw-1 |
This comment has been minimized.
This comment has been minimized.
flt2dec front of format_short & format_fixed try-job: test-x86_64-mingw-1
|
@bors r+ |
This comment has been minimized.
This comment has been minimized.
flt2dec front of format_short & format_fixed The fallback to Dragon was placed in the Grisu module. As a result tests don't test what they appear to test, and some benchmarks from Grisu measured Dragon instead. The corrected values in the tests haven been verified against the Go implementation.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test for 2a5f389 failed: CI. Failed job:
|
View all comments
The fallback to Dragon was placed in the Grisu module. As a result tests don't test what they appear to test, and some benchmarks from Grisu measured Dragon instead.
The corrected values in the tests haven been verified against the Go implementation.