-
Notifications
You must be signed in to change notification settings - Fork 13.3k
make std::intrinsics functions actually be intrinsics #139916
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
rustbot has assigned @Mark-Simulacrum. Use |
This comment has been minimized.
This comment has been minimized.
0497c07
to
82d5e7a
Compare
This comment has been minimized.
This comment has been minimized.
82d5e7a
to
5a7d62b
Compare
This comment has been minimized.
This comment has been minimized.
5a7d62b
to
5d60226
Compare
I'd be interested in seeing a crater run on that, to be honest. We've pulled the rug on things that were accidentally stabilized before, even long after the fact. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me with test confirmed to be adjusted as expected
tests/mir-opt/lower_intrinsics.f_copy_nonoverlapping.LowerIntrinsics.panic-abort.diff
Show resolved
Hide resolved
Since this is a slight breaking change (of accidentally-stable API surface), does it need an FCP of some sort -- from @rust-lang/libs-api, I assume? |
FCP since this is very slightly breaking, though unlikely to cause any issues in practice. @rfcbot merge |
Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
@bors try |
make std::intrinsic functions actually be intrinsics Most of the functions in `std::intrinsics` are actually intrinsics, but some are not: for historical reasons, `std::intrinsics::{copy,copy_nonoverlapping,write_bytes}` are accessible on stable, and the versions in `std::ptr` are just re-exports. These functions are not intrinsics, but wrappers around the intrinsic, because they add extra debug assertions. This PR makes the functions in `std::intrinsics` actually be intrinsics. - The advantage is that we can now use it in tests that need to directly call the intrinsic, thus removing a footgun for compiler development. We also remove the extended user-facing doc comments of these functions out of a file that should be largely internal documentation. - The downside is that if users are using those functions directly, they will not get the debug assertions any more. Note however that those users are already ignoring a deprecation warning, so I think this is fine. Furthermore, if someone imports the `intrinsic` name of this function and turns that into a function pointer, that will no longer work, since only the wrapper functions can be turned into a function pointer. I would be rather surprised if anyone did this, though... and again, they must have already ignored a deprecation warning. Still, seems worth a crater run, if there's general agreement that we want to go ahead with this change. (`intrinsics::drop_in_place` also remains not-an-intrinsic, which bugs me, but oh well, not much we can do about it; we can't remove it from the module as the path is accidentally-stable.) Cc `@rust-lang/libs-api` `@saethlin`
☀️ Try build successful - checks-actions |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
@bors r=Mark-Simulacrum |
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message
|
5d60226
to
a29756d
Compare
@bors r=Mark-Simulacrum |
☀️ 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 a8e4c68 (parent) -> f8e9e76 (this PR) Test differencesShow 404 test diffs404 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard f8e9e7636aabcbc29345d9614432d15b3c0c4ec7 --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 (f8e9e76): 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 (primary 0.6%, secondary 0.1%)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.7%)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 sizeResults (primary -0.1%)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.
Bootstrap: 776.444s -> 775.927s (-0.07%) |
Most of the functions in
std::intrinsics
are actually intrinsics, but some are not: for historical reasons,std::intrinsics::{copy,copy_nonoverlapping,write_bytes}
are accessible on stable, and the versions instd::ptr
are just re-exports. These functions are not intrinsics, but wrappers around the intrinsic, because they add extra debug assertions.This PR makes the functions in
std::intrinsics
actually be intrinsics.intrinsic
name of this function and turns that into a function pointer, that will no longer work, since only the wrapper functions can be turned into a function pointer. I would be rather surprised if anyone did this, though... and again, they must have already ignored a deprecation warning. Still, seems worth a crater run, if there's general agreement that we want to go ahead with this change.(
intrinsics::drop_in_place
also remains not-an-intrinsic, which bugs me, but oh well, not much we can do about it; we can't remove it from the module as the path is accidentally-stable.)Cc @rust-lang/libs-api @saethlin