Skip to content

flt2dec front of format_short & format_fixed - #154505

Open
pascaldekloe wants to merge 5 commits into
rust-lang:mainfrom
pascaldekloe:flt2dec-uni
Open

flt2dec front of format_short & format_fixed#154505
pascaldekloe wants to merge 5 commits into
rust-lang:mainfrom
pascaldekloe:flt2dec-uni

Conversation

@pascaldekloe

@pascaldekloe pascaldekloe commented Mar 28, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

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.

@rustbot

rustbot commented Mar 28, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred in float parsing

cc @tgross35

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 28, 2026
@rustbot

rustbot commented Mar 28, 2026

Copy link
Copy Markdown
Collaborator

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @scottmcm, libs
  • @scottmcm, libs expanded to 8 candidates
  • Random selection from Mark-Simulacrum, joboet, scottmcm

@pascaldekloe pascaldekloe mentioned this pull request Mar 28, 2026
Open
@tgross35 tgross35 assigned tgross35 and unassigned scottmcm Mar 28, 2026
tgross35
tgross35 reviewed Mar 28, 2026
View reviewed changes

@tgross35 tgross35 left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

View changes since this review

Comment thread library/core/src/fmt/float.rs
Comment thread library/core/src/num/imp/flt2dec/mod.rs
Comment thread library/core/src/num/imp/flt2dec/strategy/grisu.rs Outdated
Comment thread library/core/src/num/imp/flt2dec/mod.rs
Comment thread library/coretests/tests/num/flt2dec/strategy/grisu.rs Outdated
Comment thread library/coretests/benches/num/flt2dec/strategy/grisu.rs
@pascaldekloe

pascaldekloe commented Mar 29, 2026

Copy link
Copy Markdown
Contributor Author

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.

tgross35
tgross35 reviewed Mar 29, 2026
View reviewed changes

@tgross35 tgross35 left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Next batch, mostly for the second commit

View changes since this review

Comment thread library/coretests/benches/num/flt2dec/strategy/grisu.rs
Comment thread library/coretests/tests/num/flt2dec/equiv.rs
Comment thread library/coretests/tests/fmt/float.rs
Comment thread library/coretests/tests/fmt/float.rs
Comment thread library/coretests/tests/num/flt2dec/random.rs Outdated
Comment thread library/coretests/tests/num/flt2dec/equiv.rs
Comment thread library/coretests/tests/num/flt2dec/random.rs
tgross35
tgross35 reviewed Apr 27, 2026
View reviewed changes
Comment thread library/coretests/tests/num/flt2dec/equiv.rs
Comment thread library/coretests/tests/num/flt2dec/mod.rs Outdated
Comment thread library/core/src/num/imp/flt2dec/mod.rs
Comment thread library/coretests/tests/num/flt2dec/mod.rs
Comment thread library/coretests/tests/num/flt2dec/strategy/dragon.rs
Comment thread library/coretests/tests/num/flt2dec/mod.rs Outdated
Comment thread library/coretests/tests/num/flt2dec/mod.rs Outdated
Comment thread library/coretests/tests/fmt/float.rs
Comment thread library/coretests/tests/num/flt2dec/mod.rs Outdated
Comment thread library/core/src/num/imp/flt2dec/mod.rs
tgross35
tgross35 reviewed Apr 27, 2026
View reviewed changes
Comment thread library/core/src/fmt/float.rs
@tgross35

tgross35 commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Let's make sure there's no perf effect.

@bors try @rust-timer queue

@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 27, 2026
@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 27, 2026
@rustbot

rustbot commented Apr 27, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rust-bors

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 27, 2026
rust-bors Bot pushed a commit that referenced this pull request Apr 27, 2026
@bors
Auto merge of #154505 - pascaldekloe:flt2dec-uni, r=<try>
cb7a7f2 
flt2dec front of format_short & format_fixed
@rust-bors

rust-bors Bot commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: cb7a7f2 (cb7a7f2a841fd88e9e55ffb325dfabdc4476a5b2, parent: 4933094f25ba1edf2de0a203665976832fc85bc9)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer

rust-timer commented Apr 27, 2026

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (cb7a7f2): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking 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 count

This 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.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 3.9% [1.8%, 6.0%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Cycles

This perf run didn't have relevant results for this metric.

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 488.817s -> 487.773s (-0.21%)
Artifact size: 393.48 MiB -> 393.50 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 27, 2026
@pascaldekloe

pascaldekloe commented May 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

All done with a few options open @tgross35.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 14, 2026
@tgross35

tgross35 commented May 18, 2026

Copy link
Copy Markdown
Contributor

Also, you've mentioned a few times that you have followup work planned. Could you share the bigger picture or goal you're working toward? That would probably help me understand some of the changes.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@pascaldekloe
test of flt2dec::format_short & flt2dec::format_fixed normalized
1dd3139 
* No generics abstraction of Grisu and Dragon
* The stringify! of input in assertion message
* TestableFloat abstraction omitted with macro
* MSVC omission direct on respective test
* FIX test missed verification of the last digit
* FIX test assertion value miss due the previous

Unified signature plus description on assertion tools:
* check_shortest! -> check_short!
* try_exact! -> check_fixed_buf_match!
* try_fixed! -> check_fixed_resolution!
* check_exact & check_exact! -> check_fixed_mix!
* check_exact_one -> check_coef_pow2!
@pascaldekloe
typo in test value from round-to-even in fmt
45c4fe8 
Inverted assertion order. It's not the value of exp which steers the branche.
The concern at hand is not having any digits (i.e, an empty buffer). The reason
for not having digits can be asserted, as an optional double check, just to be
more sure we ended up without digits for the correct reason.

Both the current and the correct value are equivalent when rounded to three decimals.
@pascaldekloe
flt2dec/random.rs renamed to flt2dec/equiv.rs
4f10769
@pascaldekloe

pascaldekloe commented May 25, 2026

Copy link
Copy Markdown
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 25, 2026
@pascaldekloe

pascaldekloe commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

Also, you've mentioned a few times that you have followup work planned. Could you share the bigger picture or goal you're working toward? That would probably help me understand some of the changes.

We decided to proceed in small steps as the explanation didn't work. Shortest answer would be less code, fewer types, and better documentation. The longer answer is optimization. The mikado construction prevents many options with entanglement.

@pascaldekloe

pascaldekloe commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

It has been another month @tgross35.

tgross35
tgross35 approved these changes Aug 13, 2026
View reviewed changes

@tgross35 tgross35 left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Again I'm sorry this has lingered, I've not been active in libs reviews. In the future if you don't hear back from a reviewer in a few weeks (myself or anyone), feel free to @rustbot reroll to request a new one - or ask on Zulip.

I'd appreciate a ping but I expect you'll have a different reviewer for all followup work.

Anyway with the tests added back this LGTM. I'm going to rebase it to kick of PR CI and make sure nothing regressed in the meantime, r=me after then.

View changes since this review

@tgross35

tgross35 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Actually it doesn't let me push so we'll just do this

@bors try jobs=x86_64-gnu,tidy,

@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Aug 13, 2026
@bors
Auto merge of #154505 - pascaldekloe:flt2dec-uni, r=<try>
c6a6db4 
flt2dec front of format_short & format_fixed


try-job: x86_64-gnu
try-job: tidy
try-job:
@tgross35

tgross35 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

@bors r+ rollup=iffy

@rust-bors

rust-bors Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 4f10769 has been approved by tgross35

It is now in the queue for this repository.

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2026
@rust-bors

rust-bors Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: c6a6db4 (c6a6db41c78a9ad81d174bc4b944ee0a41990700)
Base parent: 79ef636 (79ef636a60b0f5ca061b09122bbbca3c7b4a3b70)

@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Aug 13, 2026
@bors
Auto merge of #154505 - pascaldekloe:flt2dec-uni, r=tgross35
a42d7c5 
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.
@rust-bors rust-bors Bot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 13, 2026
@rust-bors

rust-bors Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

💔 Test for a42d7c5 failed: CI. Failed job:

@rust-log-analyzer

rust-log-analyzer commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

The job pr-check-2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@tgross35

tgross35 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

@rustbot author

needs an update for the above failure (new clippy lint)

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tgross35 tgross35 tgross35 approved these changes

Assignees

@tgross35 tgross35

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@pascaldekloe @rustbot @tgross35 @rust-timer @rust-log-analyzer @scottmcm