Rename RawPtr::to_option() to RawPtr::as_ref()
#16634
Conversation
apoelstra
changed the title
RawPtr::to_option() to RawPtr::as_ref(), add corresponding `a...RawPtr::to_option() to RawPtr::as_ref()
apoelstra
force-pushed
the
to-option-fix
branch
from
df5e6de to
358a162
Compare
| /// that this is still an unsafe operation because the returned value could | ||
| /// be pointing to invalid memory. | ||
| unsafe fn to_option(&self) -> Option<&T>; | ||
| /// While this method and its mutable counterpart ary useful for null-safety, |
apoelstra
force-pushed
the
to-option-fix
branch
2 times, most recently
from
c781bae to
2aeb3df
Compare
| @@ -293,13 +303,22 @@ impl<T> RawPtr<T> for *const T { | |||
| } | |||
|
|
|||
| #[inline] | |||
| unsafe fn to_option(&self) -> Option<&T> { | |||
There was a problem hiding this comment.
Why aren't you preserving and deprecating this with a migration path message?
There was a problem hiding this comment.
Because I wasn't thinking :) will fix.
Edit: This is fixed above. I'm not sure why the line note hasn't disappeared.
apoelstra
force-pushed
the
to-option-fix
branch
from
2aeb3df to
36ff27c
Compare
|
@apoelstra I'm not an expert at this stuff, but I believe for various "magic" built-in types, these traits are necessary to even define methods on them. Thus you may have to introduce a |
|
@apoelstra Ugh sorry I let this slip through the cracks, rebasing makes PRs confusing as hell, and I thought you hadn't updated this. Other than the typo, this all basically seems sane, and is something I've been craving for a while, so a big +1 from me. You're gonna need to convince one of the big kids to check this out, though. |
|
@aturon Does this work for you? |
apoelstra
force-pushed
the
to-option-fix
branch
from
36ff27c to
314c2cf
Compare
|
(just fix the validite->validity typo @gankro mentioned) |
| /// While this method and its mutable counterpart are useful for null-safety, | ||
| /// it is important to note that this is still an unsafe operation because | ||
| /// the returned value could be pointing to invalid memory. | ||
| unsafe fn as_ref<'a>(&self) -> Option<&'a T>; |
There was a problem hiding this comment.
It's a bit more clear to write this as:
unsafe fn as_ref(&self) -> Option<&'static T>;
|
Looks good to me, modulo some minor nits about the signature; I'll merge once it's updated. Thanks @apoelstra! |
|
@aturon Using Or maybe this is a bug in lifetime ordering? Edit: Oh, the actual error is |
As outlined in https://aturon.github.io/style/naming/conversions.html `to_` functions names should only be used for expensive operations. Thus `to_option` is better named `as_option`. Also, putting type names into method names is considered bad style; what the user is really trying to get is a reference. This `as_ref` is even better. Also, we are missing a mutable version of this method. So add a new trait `RawMutPtr` with a corresponding `as_mut` methode. Finally, there is a bug in the signature of `to_option` which has been around since lifetime elision: originally the returned reference had 'static lifetime, but since the elision changes this become the lifetime of the raw pointer (which does not make sense, since the pointer lifetime and referent lifetime are unrelated). Fix the bug to return a reference with a fresh lifetime (which will be inferred from the calling context). [breaking-change]
apoelstra
force-pushed
the
to-option-fix
branch
from
314c2cf to
00ff5aa
Compare
|
Rebase so that the new @aturon Do you stil want me to add |
|
@apoelstra Sorry, I missed your earlier message. It's fine with me to not use |
As outlined in https://aturon.github.io/style/naming/conversions.html `to_` functions names should only be used for expensive operations. Thus `to_option` is better named `as_option`. Also, putting type names into method names is considered bad style; what the user is really trying to get is a reference. This `as_ref` is even better. Also, we are missing a mutable version of this method. Finally, there is a bug in the signature of `to_option` which has been around since lifetime elision: originally the returned reference had 'static lifetime, but since the elision changes this become the lifetime of the raw pointer (which does not make sense, since the pointer lifetime and referent lifetime are unrelated). We fix the bug to return a reference with a fresh lifetime which will be inferred from the calling context. [breaking-change]
As outlined in
https://aturon.github.io/style/naming/conversions.html
to_functions names should only be used for expensive operations.Thus
to_optionis better namedas_option. Also, putting typenames into method names is considered bad style; what the user is
really trying to get is a reference. This
as_refis even better.Also, we are missing a mutable version of this method.
Finally, there is a bug in the signature of
to_optionwhich hasbeen around since lifetime elision: originally the returned reference
had 'static lifetime, but since the elision changes this become
the lifetime of the raw pointer (which does not make sense, since
the pointer lifetime and referent lifetime are unrelated). We fix
the bug to return a reference with a fresh lifetime which will be
inferred from the calling context.
[breaking-change]