core_float_math: Move functions to math module
#141282
Merged
+990
−933
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
DJMcNab
commented
May 20, 2025
| @@ -1,3 +1,4 @@ | |||
| use core::f64; | |||
There was a problem hiding this comment.
Previously, these functions were actually testing the inherent methods on f64, as defined in std, rather than the functions from the f64 module.
Practically, that's the same thing, but the use of UFCS made that not clear.
There was a problem hiding this comment.
While you're here, mind making the couple of below imports also come from core here rather than std? That would make it match f32.
There was a problem hiding this comment.
Sure - that's written (but not yet committed). One thing to note is that these tests do still depend on std, in particular for the powf function.
There was a problem hiding this comment.
Indeed, hopefully we can get that one in soon :)
tgross35
requested changes
May 20, 2025
| @@ -1,3 +1,4 @@ | |||
| use core::f64; | |||
There was a problem hiding this comment.
While you're here, mind making the couple of below imports also come from core here rather than std? That would make it match f32.
rustbot
added
S-waiting-on-author
DJMcNab
force-pushed
the
core-float-math-math
branch
from
a51161b to
8d2f63d
Compare
|
r? @tgross35 |
|
Requested reviewer is already assigned to this pull request. Please choose another assignee. |
rustbot
added
S-waiting-on-review
DJMcNab
changed the title
core_float_math: Move functions to math foldercore_float_math: Move functions to math module
DJMcNab
force-pushed
the
core-float-math-math
branch
from
8d2f63d to
bdc66a4
Compare
This comment has been minimized.
This comment has been minimized.
When these functions were added in rust-lang#138087 It made a relatively common pattern for emulating these functions using an extension trait (which internally uses `libm`) much more fragile. If `core::f32` happened to be imported by the user (to access a constant, say), then that import in the module namespace would take precedence over `f32` in the type namespace for resolving these functions, running headfirst into the stability attribute. We ran into this in Color - https://github.com/linebender/color - and chose to release the remedial 0.3.1 and 0.2.4, to allow downstream crates to build on `docs.rs`. As these methods are perma-unstable, moving them into a new module should not have any long-term concerns, and ensures that this breakage doesn't adversely impact anyone else.
DJMcNab
force-pushed
the
core-float-math-math
branch
from
bdc66a4 to
f6709bb
Compare
tgross35
approved these changes
May 20, 2025
|
Thank you! @bors r+ |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 20, 2025
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#140972 (Add TRACING_ENABLED to Machine and add enter_trace_span!()) - rust-lang#141282 (`core_float_math`: Move functions to `math` module) - rust-lang#141288 (Get rid of unnecessary `BufDisplay` abstraction) - rust-lang#141289 (use `Self` alias in self types rather than manually substituting it) - rust-lang#141291 (link tracking issue in explicit-extern-abis.md) - rust-lang#141294 (triagebot: ping me if rustdoc js is modified) - rust-lang#141303 (Fix pagetoc inactive color in rustc book) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 21, 2025
Rollup merge of rust-lang#141282 - DJMcNab:core-float-math-math, r=tgross35 `core_float_math`: Move functions to `math` module When these functions were added in rust-lang#138087 It made a relatively common pattern for emulating these functions using an extension trait (which internally uses `libm`) much more fragile. If `core::f32` happened to be imported by the user (to access a constant, say), then that import in the module namespace would take precedence over the `f32` in the type namespace for resolving these functions, running headfirst into the stability attribute. We ran into this in [Color](https://github.com/linebender/color) and chose to release the remedial 0.3.1 and 0.2.4, to allow downstream crates to build on `docs.rs`. As these methods are perma-unstable, moving them into a new module should not have any long-term concerns, and ensures that this "breakage" doesn't adversely impact anyone else. I believe that I've made the module unstable correctly. I presume that this does not require a test to make sure stable code can't depend on the module existing? I've left the stability attribute on each function - happy to tweak this if a different pattern is more correct. Tracking issue for `core_float_math`: rust-lang#137578. This PR is as requested in rust-lang#138087. r? `@tgross35` Recommended reviewing with whitespace hidden. (This is my first PR to `std/core`/this repository, as far as I can remember)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When these functions were added in #138087 It made a relatively common pattern for emulating these functions using an extension trait (which internally uses
libm) much more fragile. Ifcore::f32happened to be imported by the user (to access a constant, say), then that import in the module namespace would take precedence over thef32in the type namespace for resolving these functions, running headfirst into the stability attribute.We ran into this in Color and chose to release the remedial 0.3.1 and 0.2.4, to allow downstream crates to build on
docs.rs.As these methods are perma-unstable, moving them into a new module should not have any long-term concerns, and ensures that this "breakage" doesn't adversely impact anyone else.
I believe that I've made the module unstable correctly. I presume that this does not require a test to make sure stable code can't depend on the module existing?
I've left the stability attribute on each function - happy to tweak this if a different pattern is more correct.
Tracking issue for
core_float_math: #137578.This PR is as requested in #138087.
r? @tgross35
Recommended reviewing with whitespace hidden.
(This is my first PR to
std/core/this repository, as far as I can remember)