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.
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
Implement ptr_as_ref_unchecked #122492
Implement ptr_as_ref_unchecked #122492
Changes from all commits
d6a1b36
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I realize this is copied from elsewhere but it has been copied so often with subtle changes of the context around it that by being a copy of a copy of a copy it reads fairly jarringly in this new case. I would prefer it was more explicit to explain that you, the caller of this
unsafe fn
, define the'a
lifetime by calling this (you may pass an explicit generic via turbofish, or it will be inferred). You are making a claim that you will uphold it for that long, and "arbitrarily chosen" makes it sound more... impersonal.All of those requirements, except mutation (which is stated in this clause) are stated previously, so perhaps it would be better to move the non-mutation clause into its own bullet. Then it can simply say "all of the previous rules".
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.
In practice this is almost always chosen by inference, which makes it equivalent to
'static
, so "arbitrarily chosen" is fairly close to the truth. Whatever new wording we use here should preserve that sentiment.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.
If I understand correctly it is still illegal to mutate
&'a Inner
if I use this to obtain&'a Inner
directly, instead of&'a UnsafeCell<Inner>
. It might be better to describe this as "except via pointers obtained byUnsafeCell::get
and other functions onUnsafeCell
." Again, I understand this has already been copied a zillion times and you didn't originate this.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.
A lot of doctests don't really test anything because they're
no_run
or they only have prints like this one. Perhaps this could test something simple likeThere 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.
Are we sure we should be associating this with const-stability of this set of fn instead of just landing these as const stable when we land them? Can you make sure the two issues are crosslinked and mention that as an option?
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.
Indeed we shouldn't associate this with const_ptr_as_ref, so I'm adjusting that in #130164.
When a
const fn
is still#[unstable]
, it should generally use the same feature to track its regular stability and const-stability. Then when that feature moves towards stabilization we can decide whether the const-ness can be stabilized as well, or whether it should be moved into a new feature.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.
My comments from before apply down the line here.
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.
My comments from before etc.
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.
Why are we hiding the
assert_eq!
here?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.
Genuine copypasta
https://github.com/rust-lang/rust/pull/122492/files#diff-d0f76807985a6a8aa06787f524381d47762ce707757608c3f538b59abeebb0faL684
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.
ah, delicious italian food