-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Initial implementation of core_float_math
#138087
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment has been minimized.
This comment has been minimized.
9cc8ffd
to
b74488e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Initial implementation of `core_float_math` Since [1], `compiler-builtins` makes a certain set of math symbols weakly available on all platforms. This means we can begin exposing some of the related functions in `core`, so begin this process here. It is not possible to provide inherent methods in both `core` and `std` while giving them different stability gates, so standalone functions are added instead. This provides a way to experiment with the functionality while unstable; once it is time to stabilize, they can be converted to inherent. For `f16` and `f128`, everything is unstable so we can move the inherent methods. The following are included to start: * floor * ceil * round * round_ties_even * trunc * fract * mul_add * div_euclid * rem_euclid * powi * sqrt * abs_sub * cbrt These mirror the set of functions that we have in `compiler-builtins` since [1]. Tracking issue: rust-lang#137578 [1]: rust-lang/compiler-builtins#763 r? `@ghost` try-job: aarch64-gnu try-job: arm-android tru-job: armhf-gnu try-job: test-various try-job: x86_64-apple-1 try-job: aarch64-apple try-job: i686-msvc-1 try-job: x86_64-msvc-ext2
This comment was marked as outdated.
This comment was marked as outdated.
b74488e
to
12ad9b8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Initial implementation of `core_float_math` Since [1], `compiler-builtins` makes a certain set of math symbols weakly available on all platforms. This means we can begin exposing some of the related functions in `core`, so begin this process here. It is not possible to provide inherent methods in both `core` and `std` while giving them different stability gates, so standalone functions are added instead. This provides a way to experiment with the functionality while unstable; once it is time to stabilize, they can be converted to inherent. For `f16` and `f128`, everything is unstable so we can move the inherent methods. The following are included to start: * floor * ceil * round * round_ties_even * trunc * fract * mul_add * div_euclid * rem_euclid * powi * sqrt * abs_sub * cbrt These mirror the set of functions that we have in `compiler-builtins` since [1]. Tracking issue: rust-lang#137578 [1]: rust-lang/compiler-builtins#763 r? `@ghost` try-job: aarch64-apple try-job: aarch64-gnu try-job: arm-android tru-job: armhf-gnu try-job: dist-various-1 try-job: dist-various-2 try-job: i686-msvc-1 try-job: test-various try-job: x86_64-apple-1 try-job: x86_64-msvc-ext2
This comment was marked as outdated.
This comment was marked as outdated.
12ad9b8
to
8d57e5a
Compare
This comment has been minimized.
This comment has been minimized.
8d57e5a
to
eabddae
Compare
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Initial implementation of `core_float_math` Since [1], `compiler-builtins` makes a certain set of math symbols weakly available on all platforms. This means we can begin exposing some of the related functions in `core`, so begin this process here. It is not possible to provide inherent methods in both `core` and `std` while giving them different stability gates, so standalone functions are added instead. This provides a way to experiment with the functionality while unstable; once it is time to stabilize, they can be converted to inherent. For `f16` and `f128`, everything is unstable so we can move the inherent methods. The following are included to start: * floor * ceil * round * round_ties_even * trunc * fract * mul_add * div_euclid * rem_euclid * powi * sqrt * abs_sub * cbrt These mirror the set of functions that we have in `compiler-builtins` since [1], with the exception of `powi` that has been there longer. Tracking issue: rust-lang#137578 [1]: rust-lang/compiler-builtins#763 r? `@ghost` try-job: aarch64-apple try-job: aarch64-gnu try-job: arm-android tru-job: armhf-gnu try-job: dist-various-1 try-job: dist-various-2 try-job: i686-msvc-1 try-job: test-various try-job: x86_64-apple-1 try-job: x86_64-msvc-ext2
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
4375018
to
77794c6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Initial implementation of `core_float_math` Since [1], `compiler-builtins` makes a certain set of math symbols weakly available on all platforms. This means we can begin exposing some of the related functions in `core`, so begin this process here. It is not possible to provide inherent methods in both `core` and `std` while giving them different stability gates, so standalone functions are added instead. This provides a way to experiment with the functionality while unstable; once it is time to stabilize, they can be converted to inherent. For `f16` and `f128`, everything is unstable so we can move the inherent methods. The following are included to start: * floor * ceil * round * round_ties_even * trunc * fract * mul_add * div_euclid * rem_euclid * powi * sqrt * abs_sub * cbrt These mirror the set of functions that we have in `compiler-builtins` since [1], with the exception of `powi` that has been there longer. Tracking issue: rust-lang#137578 [1]: rust-lang/compiler-builtins#763 r? `@ghost` try-job: aarch64-apple try-job: aarch64-gnu try-job: arm-android tru-job: armhf-gnu try-job: dist-various-1 try-job: dist-various-2 try-job: i686-msvc-1 try-job: test-various try-job: x86_64-apple-1 try-job: x86_64-msvc-ext2
test-various passed, the others likely only need tweaks for f128 config. r? @Amanieu |
Discussed in person @bors r=Amanieu |
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing bf5a38d (parent) -> 777d372 (this PR) Test differencesShow 1962 test diffsStage 0
Stage 1
(and 504 additional test diffs) Additionally, 1358 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 777d372772aa3b39ba7273fcb8208a89f2ab0afd --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (777d372): comparison URL. Overall result: ❌✅ regressions and improvements - no action needed@rustbot label: -perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (secondary -1.4%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 0.6%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 774.349s -> 775.816s (0.19%) |
This landing in nightly has unfortunately broken Linebender's Name resolution seems to prefer these new functions over the ones defined on the I'm not sure how to determine if this class of breakage is classed as allowable. I think it would in theory be a regression on stable once 1.89 is stable. However, no matter where these functions live, it would be possible to adversarially trigger this for any unstable function being added. |
Are you glob importing something from std? This is something we do try to discourage because that means that std adding any new api can break things (rust-lang/rust-clippy#13961). Regardless of the outcome here, please consider fixing this in However, this is also intended to be temporary API that isn’t worth breaking anybody over, and |
For reference – fix for color: linebender/color#175 |
For reference, all the primitive types are re-exported in |
No, not that I'm aware of. We're only importing the (module) We have now made a remedial release of color (I don't know if this impacts prior minor versions - I'm still on mobile). |
It's hard to evaluate the breakage without the crater run, so we should either:
|
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.
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.
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.
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.
…ross35 `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)
Thanks for clarifying, that is an unusual case.
The new module seems easier than bothering to evaluate breakage, especially since this is destined to get deleted anyway. #141282 moves them. |
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)
Initial implementation of `core_float_math` Since [1], `compiler-builtins` makes a certain set of math symbols weakly available on all platforms. This means we can begin exposing some of the related functions in `core`, so begin this process here. It is not possible to provide inherent methods in both `core` and `std` while giving them different stability gates, so standalone functions are added instead. This provides a way to experiment with the functionality while unstable; once it is time to stabilize, they can be converted to inherent. For `f16` and `f128`, everything is unstable so we can move the inherent methods. The following are included to start: * floor * ceil * round * round_ties_even * trunc * fract * mul_add * div_euclid * rem_euclid * powi * sqrt * abs_sub * cbrt These mirror the set of functions that we have in `compiler-builtins` since [1], with the exception of `powi` that has been there longer. Details for each of the changes is in the commit messages. Tracking issue: rust-lang#137578 [1]: rust-lang/compiler-builtins#763 try-job: aarch64-gnu tru-job: armhf-gnu try-job: i686-msvc-1 try-job: test-various try-job: x86_64-mingw-1 try-job: x86_64-mingw-2
Since 1,
compiler-builtins
makes a certain set of math symbolsweakly available on all platforms. This means we can begin exposing some
of the related functions in
core
, so begin this process here.It is not possible to provide inherent methods in both
core
andstd
while giving them different stability gates, so standalone functions are
added instead. This provides a way to experiment with the functionality
while unstable; once it is time to stabilize, they can be converted to
inherent.
For
f16
andf128
, everything is unstable so we can move the inherentmethods.
The following are included to start:
These mirror the set of functions that we have in
compiler-builtins
since 1, with the exception of
powi
that has been there longer.Details for each of the changes is in the commit messages.
Tracking issue: #137578
try-job: aarch64-gnu
tru-job: armhf-gnu
try-job: i686-msvc-1
try-job: test-various
try-job: x86_64-mingw-1
try-job: x86_64-mingw-2