fmt of floating points defragmented

[pascaldekloe]

• Formatting of floating-points centralised in fmt/float.rs

• Drop of numfmt feature; no intermediate Part structure

• FullDecoded step from omitted with num::FpCategory

• MaybeUninit to str contained in grisu.rs & dragon.rs

• Exponent in native bit-width (isize instead of i16)

• Decode logic of floating-points explained in comments

• Explicit fallback from Grisu to Dragon (with Option)

• Abstraction between Grisu and Dragon in tests undone

• Fixed buffer-size for formatting in "shortest" mode

• Macro instead of traits for generic handling of types

• FIX: check_exact macro missed check of last decimal

• FIX: faulty values in *_sanity_test due to previous

  r? tgross35

[rustbot]

tgross35 is currently at their maximum review capacity.
They may take a while to respond.

[pascaldekloe]

Refactor a little cascaded a little. 😅 I hope you are OK with such big change.

The new pad_number method on fmt::Formatter could be part of the core library.

[quaternic]

@pascaldekloe

Refactor a little cascaded a little. 😅 I hope you are OK with such big change.

At +1,625 −3,292 (lines), this is quite large indeed, and as such, difficult to review. That is in part due to incorporating many different kinds of changes, which makes it hard to get an idea of what is actually being changed.

For one, parts of the diff seem to be just renames of functions or local variables, which would be easy to review if that's all it did. If the renames feel necessary for clarity, it would help to do them in a separate commit.

• Formatting of floating-points centralised in fmt/float.rs

• Drop of numfmt feature; no intermediate Part structure
  ...

These look like short commit descriptions. Did you have these as separate commits at some point?

[pascaldekloe]

It was a single commit that grew into something big, @quaternic. Functions were renamed while changing their signatures. We might as well use better names when touching those lines. Many comments were rewritten while figuring out what they meant.

Logic from traits in num::flt2dec moved to the macro_rules in fmt/float.rs.
Logic from num/flt2dec/mod.rs also moved to fmt/float.rs (without use of Part).
Those two moves cause the majority of the change. It is hard to separate the modification from the relocation here.

On hindsight, I should have undone the abstraction in tests first. Especially the broken test values would have been nice to see in a separate commit.

I believe the changes in strategy (Grisu and Dragon) are readable as is.

[bors]

☔ The latest upstream changes (presumably #150334) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[rust-log-analyzer]

The job aarch64-gnu-llvm-20-1 failed! Check out the build log: (web) (plain enhanced) (plain)

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

---- [ui] tests/ui/contracts/contract-captures-via-closure-noncopy.rs stdout ----
Saved the actual stderr to `/checkout/obj/build/aarch64-unknown-linux-gnu/test/ui/contracts/contract-captures-via-closure-noncopy/contract-captures-via-closure-noncopy.stderr`
diff of stderr:

- error[E0277]: the trait bound `Baz: std::marker::Copy` is not satisfied in `{closure@$DIR/contract-captures-via-closure-noncopy.rs:13:42: 13:57}`
+ error[E0277]: the trait bound `Baz: Copy` is not satisfied in `{closure@$DIR/contract-captures-via-closure-noncopy.rs:13:42: 13:57}`
2   --> $DIR/contract-captures-via-closure-noncopy.rs:13:1
3    |
4 LL | #[core::contracts::ensures({let old = x; move |ret:&Baz| ret.baz == old.baz*2 })]

9    | unsatisfied trait bound
10    | required by a bound introduced by this call
11    |
-    = help: within `{closure@$DIR/contract-captures-via-closure-noncopy.rs:13:42: 13:57}`, the trait `std::marker::Copy` is not implemented for `Baz`
+    = help: within `{closure@$DIR/contract-captures-via-closure-noncopy.rs:13:42: 13:57}`, the trait `Copy` is not implemented for `Baz`
13 note: required because it's used within this closure
14   --> $DIR/contract-captures-via-closure-noncopy.rs:13:42
15    |

Note: some mismatched output was normalized before being compared
- error[E0277]: the trait bound `Baz: Copy` is not satisfied in `{closure@/checkout/tests/ui/contracts/contract-captures-via-closure-noncopy.rs:13:42: 13:57}`
-   --> /checkout/tests/ui/contracts/contract-captures-via-closure-noncopy.rs:13:1
-    = help: within `{closure@/checkout/tests/ui/contracts/contract-captures-via-closure-noncopy.rs:13:42: 13:57}`, the trait `Copy` is not implemented for `Baz`
+ error[E0277]: the trait bound `Baz: Copy` is not satisfied in `{closure@$DIR/contract-captures-via-closure-noncopy.rs:13:42: 13:57}`
+    = help: within `{closure@$DIR/contract-captures-via-closure-noncopy.rs:13:42: 13:57}`, the trait `Copy` is not implemented for `Baz`


The actual stderr differed from the expected stderr
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args contracts/contract-captures-via-closure-noncopy.rs`

error: 1 errors occurred comparing output.
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/aarch64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/contracts/contract-captures-via-closure-noncopy.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/aarch64-unknown-linux-gnu/stage2" "--target=aarch64-unknown-linux-gnu" "--check-cfg" "cfg(test,FALSE)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/aarch64-unknown-linux-gnu/test/ui/contracts/contract-captures-via-closure-noncopy" "-A" "unused" "-W" "unused_attributes" "-A" "internal_features" "-A" "unused_parens" "-A" "unused_braces" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/aarch64-unknown-linux-gnu/native/rust-test-helpers" "--edition=2015" "-Zcontract-checks=yes"
stdout: none
--- stderr -------------------------------
error[E0277]: the trait bound `Baz: Copy` is not satisfied in `{closure@/checkout/tests/ui/contracts/contract-captures-via-closure-noncopy.rs:13:42: 13:57}`
##[error]  --> /checkout/tests/ui/contracts/contract-captures-via-closure-noncopy.rs:13:1
   |
LL | #[core::contracts::ensures({let old = x; move |ret:&Baz| ret.baz == old.baz*2 })]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^------------------------------------^^^^
   | |                                        |
   | |                                        within this `{closure@/checkout/tests/ui/contracts/contract-captures-via-closure-noncopy.rs:13:42: 13:57}`
   | |                                        this tail expression is of type `{[email protected]:13:42}`
   | unsatisfied trait bound
   | required by a bound introduced by this call
   |
   = help: within `{closure@/checkout/tests/ui/contracts/contract-captures-via-closure-noncopy.rs:13:42: 13:57}`, the trait `Copy` is not implemented for `Baz`
note: required because it's used within this closure
  --> /checkout/tests/ui/contracts/contract-captures-via-closure-noncopy.rs:13:42
   |
LL | #[core::contracts::ensures({let old = x; move |ret:&Baz| ret.baz == old.baz*2 })]
   |                                          ^^^^^^^^^^^^^^^
note: required by a bound in `build_check_ensures`
  --> /rustc/FAKE_PREFIX/library/core/src/contracts.rs:19:0
help: consider annotating `Baz` with `#[derive(Copy)]`
   |
LL + #[derive(Copy)]
LL | struct Baz {
   |

[tgross35]

Thank you for taking a look at this!

As @quaternic mentioned, this is a large and pretty messy diff that doesn't have a lot of context. The first commit is fine, but unfortunately I don't really think the second commit is reviewable as-is. Would you be able to split it up? Doing changes approximately what you have listed in the top post would be extremely helpful.

It would also be valuable to see the reasoning in the messages: e.g. whether it's a nonfunctional cleanup, benchmark or codegen improvements you saw that motivated that change, etc. As-is, I'm not sure what the intended runtime effects of any of these changes are.

Once CI passes, we can run perf.

@rustbot author

[rustbot]

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

[tgross35]

As a complete aside, jj makes organizing commits quite a bit nicer than vanilla git. You can split a commit into smaller bits, rebase a single commit around, squash, update the messages with desc, etc on arbitrary commits, rather than always needing to go through the linear git rebase.

Also easy to do stacked PRs if you choose, since you can put a bookmark (branch) on multiple commits in a series that all get updated if the base changes.

[pascaldekloe]

It is very hard if not impossible to undo so many mistakes by using clean and comprehensible steps. That's probably the reason why we ended up with this mikado construction in the first place.

As I said, the majority of the change is a full write out of the various parts into float.rs. Maybe you could have a look at this file and tell me whether this is indeed how things should look like @tgross35?

If we can agree on the result then I will do a lot on making this work, including rewriting the whole thing into pieces if that's what it takes. Can definitely further elaborate on the issues and fixes. The summary in the description is brief and incomplete.

To be honest, I don't understand why the build is failing there. It looks unrelated. Some help would be appreciated. 🙏

[rust-bors]

☔ The latest upstream changes (presumably #150925) made this pull request unmergeable. Please resolve the merge conflicts.

[tgross35]

It is very hard if not impossible to undo so many mistakes by using clean and comprehensible steps. That's probably the reason why we ended up with this mikado construction in the first place.

Certainly not mistakes :) I've run into the same trap when touching this code, #134063 was a single blob that I had to go back and split up (though that's less than a third the size of this one).

As I said, the majority of the change is a full write out of the various parts into float.rs. Maybe you could have a look at this file and tell me whether this is indeed how things should look like @tgross35?

I've attempted to look through both the diff here and the resulting files and I still can't exactly grok what I need to be looking for, at least not within a reasonable amount of time. So instead, here's a piecemeal response to the things you've mentioned doing:

• Formatting of floating-points centralised in fmt/float.rs

I'd actually recommend going the other way here. IMO it would be better if fmt only contained:

• The types and interfaces that actually live in core::fmt, and what is needed to support them
• Trait implementations for floats and integers that immediately call out to separate functions
• nofloat since it's the same thing but panic instead of call out

Whatever currently is there for float formatting can move into num/flt2dec. Int formatting could similarly move to a new num/int2dec module, then there wouldn't be any algorithmic bits in core/fmt.

I'd recommend doing this as a standalone PR since it's wider than the rest of the changes.

* Drop of numfmt feature; no intermediate Part structure

This is replaced by more direct calls to things that are currently #[doc(hidden)] right? I'd be interested to take a closer look at what the transformation is but this sounds good to me.

* FullDecoded step from omitted with num::FpCategory

If this is squashed somewhere else then it seems fine.

* MaybeUninit to str contained in grisu.rs & dragon.rs

Also likely fine if it leads to cleaner code.

* Exponent in native bit-width (isize instead of i16)

If this doesn't gain anything then I would leave it. I see one case of i16 and one of u8, in changing to isize you lose an idea of the scale and signedness. isize::from(...) is free for either of those when it is needed.

Also in theory using the smaller types may pack better, though that likely isn't relevant with inlining.

* Decode logic of floating-points explained in comments

Comments are always a 👍

* Explicit fallback from Grisu to Dragon (with Option)

I'm not sure what this one means.

* Abstraction between Grisu and Dragon in tests undone

Again I'm not exactly sure what this one will do, but I'll note that it would be good to tread a bit cautiously here. We currently have the two algorithms but it's likely that we may have more in the future or swap them out for different cases. So if the current testing is amenable to running the same tests against different algorithms, I'd leave it more similar to what it currently is.

* Fixed buffer-size for formatting in "shortest" mode

Without knowing exactly where, this sounds reasonable.

* Macro instead of traits for generic handling of types

If it's tiny things that get simplified then this can be okay, but I'd rather avoid largeer refactoring of this kind in general. It's significantly easier to extend traits when we need more functionality than it is to extend macros.

* FIX: check_exact macro missed check of last decimal
* FIX: faulty values in *_sanity_test due to previous

If we can agree on the result then I will do a lot on making this work, including rewriting the whole thing into pieces if that's what it takes. Can definitely further elaborate on the issues and fixes. The summary in the description is brief and incomplete.

I appreciate you being willing. Thanks for putting up this PR too, this is some pretty intense code that could use a cleanup.

To be honest, I don't understand why the build is failing there. It looks unrelated. Some help would be appreciated. 🙏

Probably just something spurious that will be fixed with a rebase.