-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Less as *
in library/core
#109255
Less as *
in library/core
#109255
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
This comment was marked as resolved.
This comment was marked as resolved.
Personally I'm not the greatest fan of |
(Nominated in case libs wants to give their 2¢ on whether this is a desired change for |
This comment was marked as resolved.
This comment was marked as resolved.
`cast`/`cast_const`/`cast_mut` have been stable and const-stable for a while, so let's use them instead of `as`, for clarity about what the cast is doing and to emphasize `as` casts doing anything else. After all, if it had existed back then, using `.cast::<T>()` instead of `as *mut T` would have helped catch the soundness bug back in <https://blog.rust-lang.org/2017/02/09/Rust-1.15.1.html#whats-in-1151-stable>. I'm working on a lint to enforce this, which is how I found all these cases :) Old zulip conversation about moving things off `as` where feasible: <https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Adding.20methods.20as.20more.20specific.20versions.20of.20.60as.60/near/238374585>.
6dce717
to
6c65e34
Compare
I have some concerns about performance since this now instantiates a lot of small inline functions. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit 6c65e34 with merge e473c2a4f4c5b4fed05b510c8070ac086bcde415... |
Running perf makes sense. I'll note that these are all rust/library/core/src/ptr/const_ptr.rs Lines 107 to 110 in 439292b
(And hopefully the MIR inliner has already run by the time user code is instantiating anything that calls them.) |
☀️ Try build successful - checks-actions |
1 similar comment
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (e473c2a4f4c5b4fed05b510c8070ac086bcde415): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
|
No primary regressions, overall more improvements than regressions. @rustbot label: +perf-regression-triaged |
This was discussed last week in the library team meeting. We're all in favor of this change, but a lint for this would be nice, to make sure we avoid adding new |
This is just going to bit-rot, so I'll close it until I can get back to a broader thing. |
cast
/cast_const
/cast_mut
have been stable and const-stable for a while, so let's use them instead ofas
, for clarity about what the cast is doing and to emphasizeas
casts doing anything else.After all, if it had existed back then, using
.cast::<T>()
instead ofas *mut T
might have helped catch the soundness bug back in https://blog.rust-lang.org/2017/02/09/Rust-1.15.1.html#whats-in-1151-stable, as the copy-paste wouldn't have compiled (sincecast
would have given a*const
not a*mut
).I'm working on a lint to enforce this in future, which I used to find all these places.
Old zulip conversation about moving things off
as
where feasible: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Adding.20methods.20as.20more.20specific.20versions.20of.20.60as.60/near/238374585.