Skip to content
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.

Sign up for GitHub

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

Add as_{,mut_}ptr methods to Option #80308

Closed
wants to merge 2 commits into from
+78 −0
Closed

Add as_{,mut_}ptr methods to Option #80308

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fc15179
Add `as_{,mut_}ptr` methods to `Option`
nvzqz Dec 22, 2020
1e0627e
Add missing "mut" to docs
nvzqz Dec 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions library/core/src/option.rs
View file
Original file line number Diff line number Diff line change
@@ -152,6 +152,7 @@ use crate::pin::Pin;
use crate::{
convert, fmt, hint, mem,
ops::{self, Deref, DerefMut},
ptr,
};

/// The `Option` type. See [the module level documentation](self) for more.
@@ -985,6 +986,83 @@ impl<T> Option<T> {
}
}

impl<T> Option<&T> {
/// Converts from `Option<&T>` (or `&Option<&T>`) to `*const T`.
Copy link
Member

@m-ou-se m-ou-se Jan 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(or &Option<&T>)

Maybe this should take an Option<&T> by value instead? Or I suppose that might be problematic with the &mut versions? Edit: You already mentioned that. Never mind. :)

Regardless, it's probably better to leave out the (or `&Option<&T>`) part from the documentation, as that just might add confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added that for consistency with the docs of Option::as_deref and Option::as_deref_mut. I agree it doesn't add much value.

///
/// This is the opposite of `<*const T>::as_ref`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you turn <*const T>::as_ref into a link to its documentation?

Copy link
Contributor Author

@nvzqz nvzqz Jan 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. Do you happen to know how to intra-doc link pointer type methods? Otherwise I'll just use the full URL.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paging @jyn514, who probably knows this. ^^

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[..][pointer::as_ref] seems to work, but that doesn't give you control over which of the two as_refs it links to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it guaranteed that #method.as_ref and #method.as_ref-1 will refer to the const and mut variants respectively? If not, it might be worth adding a custom id.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pointer was recently implemented and isn't in beta yet, so you can't use it until the beta bump in 6 weeks (or else doc --stage 0 will break). #80181 (comment)

There is currently no way to distinguish *const from *mut, I wouldn't consider impl-1 stable and would prefer not to use it since the link checker won't warn if *const and *mut get swapped.

///
/// # Examples
///
/// ```
/// #![feature(option_as_ptr)]
///
/// let opt = Some(&3);
/// let ptr = opt.as_ptr();
///
/// assert_eq!(opt, unsafe { ptr.as_ref() });
/// ```
#[unstable(feature = "option_as_ptr", issue = "none")]
#[inline]
pub const fn as_ptr(&self) -> *const T {
match self {
Some(x) => *x,
None => ptr::null(),
}
}
}

impl<T> Option<&mut T> {
/// Converts from `Option<&mut T>` (or `&Option<&mut T>`) to `*const T`.
///
/// # Examples
///
/// ```
/// #![feature(option_as_ptr)]
///
/// let mut x = 3;
/// let opt = Some(&mut x);
/// let ptr = opt.as_ptr();
///
/// assert_eq!(x, unsafe { *ptr });
/// ```
#[unstable(feature = "option_as_ptr", issue = "none")]
#[rustc_const_unstable(feature = "option_as_ptr", issue = "none")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you put #[rustc_const_unstable] here, but not on the one above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because these are over &mut T, which is unstable in const fn. Whereas the one above is &T, which has no issues in const fn. Should they both be #[rustc_const_unstable] regardless?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the state of that attribute is. In the rustc source code, it says:

rust/compiler/rustc_mir/src/transform/check_consts/mod.rs

Lines 113 to 116 in 8fec6c7

// Functions with `#[unstable]` are const-unstable.
//
// FIXME(ecstaticmorse): We should keep const-stability attributes wholly separate from normal stability
// attributes. `#[unstable]` should be irrelevant.

Which doesn't really give an answer here.

I suppose it's useful to remind us we're also stabilizing the constness of the function at the point when we stabilize these new functions. So it wouldn't hurt to add it to the first one too, just in case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair point. I'll make this change then.

#[inline]
pub const fn as_ptr(&self) -> *const T {
match self {
Some(x) => *x,
None => ptr::null(),
}
}

/// Converts from `Option<&mut T>` (or `&mut Option<&mut T>`) to `*mut T`.
///
/// This is the opposite of `<*mut T>::as_mut`.
///
/// # Examples
///
/// ```
/// #![feature(option_as_ptr)]
///
/// let mut x = 3;
/// let mut opt = Some(&mut x);
///
/// let ptr = opt.as_mut_ptr();
/// unsafe { *ptr = 4 };
///
/// assert_eq!(x, 4);
/// ```
#[unstable(feature = "option_as_ptr", issue = "none")]
#[rustc_const_unstable(feature = "option_as_ptr", issue = "none")]
#[inline]
pub const fn as_mut_ptr(&mut self) -> *mut T {
match self {
Some(x) => *x,
None => ptr::null_mut(),
}
}
}

impl<T: Copy> Option<&T> {
/// Maps an `Option<&T>` to an `Option<T>` by copying the contents of the
/// option.