Add some APIs to ptr::NonNull and fix `since` attributes #47631

SimonSapin · 2018-01-21T09:02:39Z

This is a follow-up to its stabilization in #46952. Tracking issue: #27730.

These trait impls are insta-stable: Hash, PartialEq, Eq, PartialOrd and Ord.
The new cast() -> NonNull method is #[unstable]. It was proposed in Rename std::ptr::Shared to NonNull and stabilize it #46952 (comment).

rust-highfive · 2018-01-21T09:02:44Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @KodrAus (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

eddyb · 2018-01-21T09:10:31Z

src/libcore/ptr.rs

+
+    /// Cast to a pointer of another type
+    #[unstable(feature = "nonnull", issue = "27730")]
+    pub fn cast<U>(self) -> NonNull<U> {


Shouldn't this be U: ?Sized?

It should. I’ll also add some tests.

… or not:

error[E0606]: casting `*mut T` as `*mut U` is invalid = note: vtable kinds may not match

Then maybe this method should also require T: Sized?

KodrAus · 2018-01-21T09:16:14Z

Thanks @SimonSapin!

Should we also add PartialOrd and Ord? Raw pointers implement them.

I think so.

SimonSapin · 2018-01-21T09:28:11Z

General question: when should methods in libcore / libstd have #[inline]?

dtolnay · 2018-01-21T17:04:59Z

src/libcore/ptr.rs

+
+    /// Cast to a pointer of another type
+    #[unstable(feature = "nonnull_cast", issue = /* FIXME */ "0")]
+    pub fn cast<U>(self) -> NonNull<U> where T: Sized {


What's wrong with T: ?Sized here? It makes sense to me to cast the data pointer of NonNull<[u8]> to NonNull<[u8; 64]> after performing a length check.

It's asymmetric because there's no trait that models as precisely, so U can't also be unsized.

Oh you mean the called doing a length check themselves? I added where T: Sized because I assumed as would not compile without it, but it looks like it does. Updated.

SimonSapin · 2018-01-21T23:08:39Z

If cast requires more discussion I’ll split it into a separate PR so that the derives can land in Nightly. Let me know. It’s unstable though, we can always change it later.

dtolnay

The trait impls match what is already available on raw pointers so that seems non-controversial. NonNull::cast looks good to me as an unstable addition. Please create a tracking issue for cast and then this should be ready to merge! Thanks.

KodrAus · 2018-01-22T03:10:49Z

General question: when should methods in libcore / libstd have #[inline]?

@SimonSapin My understanding is we should add the #[inline] attribute to public non-generic functions/methods that we expect to get inlined, because generic functions are monomorphised in the consuming crate so they're candidates for inlining, but non generic functions aren't. At least that's roughly how @sfackler explained it to me, there might be more to it though.

For example in a quick test in a library the following do/don't seem to get inlined in a consuming binary:

// Inlined
pub fn generic<T>(t: T) -> T { t }

// Inlined
#[inline]
pub fn generic_inline<T>(t: T) -> T { t }

// Not inlined
pub fn non_generic(i: i32) -> i32 { i + 10 }

// Inlined
#[inline]
pub fn non_generic_inline(i: i32) -> i32 { i + 10 }

impl GenericTrait<i32> for i32 {
    // Not inlined
    fn generic(self) -> i32 { self }

    // Inlined
    #[inline]
    fn generic_inline(self) -> i32 { self }

    // Inlined
    fn generic_method<U>(self, u: U) -> (i32, U) { (self, u) }

    // Inlined
    #[inline]
    fn generic_method_inline<U>(self, u: U) -> (i32, U) { (self, u) }
}

pub trait GenericTrait<T> {
    fn generic(self) -> T;
    fn generic_inline(self) -> T;
    fn generic_method<U>(self, u: U) -> (T, U);
    fn generic_method_inline<U>(self, u: U) -> (T, U);
}

KodrAus · 2018-01-22T03:16:27Z

This looks good to me too!

@rfcbot fcp merge

rfcbot · 2018-01-22T03:16:27Z

Team member @KodrAus has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, 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.

This is less verbose than going through raw pointers to cast with `as`.

SimonSapin · 2018-01-22T08:22:57Z

@dtolnay Done: #47653

kennytm · 2018-01-31T12:30:01Z

Triage ping, ticky boxes for you @BurntSushi!

pietroalbini · 2018-02-05T10:57:19Z

@BurntSushi there is a nice checkbox in #47631 (comment) waiting for you!

rfcbot · 2018-02-06T21:22:44Z

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot · 2018-02-06T21:22:44Z

🔔 This is now entering its final comment period, as per the review above. 🔔

alexcrichton · 2018-02-06T21:29:43Z

@bors: r+

bors · 2018-02-06T21:29:44Z

📌 Commit b8ffc8a has been approved by alexcrichton

bors · 2018-02-07T10:18:42Z

⌛ Testing commit b8ffc8a with merge 161ca1128f172a843e19cab030c6cb2c9e06c6f4...

bors · 2018-02-07T13:19:15Z

💔 Test failed - status-travis

kennytm · 2018-02-07T13:56:48Z

@bors retry rollup

3 hour timeout...

Add some APIs to ptr::NonNull and fix `since` attributes This is a follow-up to its stabilization in rust-lang#46952. Tracking issue: rust-lang#27730. * These trait impls are insta-stable: `Hash`, `PartialEq`, `Eq`, `PartialOrd` and `Ord`. * The new `cast() -> NonNull` method is `#[unstable]`. It was proposed in rust-lang#46952 (comment).

Rollup of 10 pull requests - Successful merges: #47613, #47631, #47810, #47883, #47922, #47944, #48014, #48018, #48020, #48028 - Failed merges:

By reversing the arguments we achieve several clarifications: - The function closely resembles `cast` but with an argument to initialized the metadata. This is easier to teach and answers an long outstanding question that had restricted cast to `Sized` targets initially. See multiples reviews of <rust-lang#47631> - The 'object identity', in the form or provenance, is now preserved from the call receiver to the result. This helps explain the method as a builder-style, instead of some kind of setter that would modify something in-place. Ensuring that the result has the identity of the `self` argument is also beneficial for an intuition of effects. - An outstanding concern, 'Correct argument type', is avoided by not committing to any specific argument type. This is consistent with cast which does not require its receiver to be a raw address.

Refactor set_ptr_value as with_metadata_of Replaces `set_ptr_value` (rust-lang#75091) with methods of reversed argument order: ```rust impl<T: ?Sized> *mut T { pub fn with_metadata_of<U: ?Sized>(self, val: *mut U) -> *mut U; } impl<T: ?Sized> *const T { pub fn with_metadata_of<U: ?Sized>(self, val: *const U) -> *const U; } ``` By reversing the arguments we achieve several clarifications: - The function closely resembles `cast` with an argument to initialize the metadata. This is easier to teach and answers a long outstanding question that had restricted cast to `Sized` pointee targets. See multiples reviews of <rust-lang#47631> - The 'object identity', in the form of provenance, is now preserved from the receiver argument to the result. This helps explain the method as a builder-style, instead of some kind of setter that would modify something in-place. Ensuring that the result has the identity of the `self` argument is also beneficial for an intuition of effects. - An outstanding concern, 'Correct argument type', is avoided by not committing to any specific argument type. This is consistent with cast which does not require its receiver to be a 'raw address'. Hopefully the usage examples in `sync/rc.rs` serve as sufficient examples of the style to convince the reader of the readability improvements of this style, when compared to the previous order of arguments. I want to take the opportunity to motivate inclusion of this method _separate_ from metadata API, separate from `feature(ptr_metadata)`. It does _not_ involve the `Pointee` trait in any form. This may be regarded as a very, very light form that does not commit to any details of the pointee trait, or its associated metadata. There are several use cases for which this is already sufficient and no further inspection of metadata is necessary. - Storing the coercion of `*mut T` into `*mut dyn Trait` as a way to dynamically cast some an arbitrary instance of the same type to a dyn trait instance. In particular, one can have a field of type `Option<*mut dyn io::Seek>` to memorize if a particular writer is seekable. Then a method `fn(self: &T) -> Option<&dyn Seek>` can be provided, which does _not_ involve the static trait bound `T: Seek`. This makes it possible to create an API that is capable of utilizing seekable streams and non-seekable streams (instead of a possible less efficient manner such as more buffering) through the same entry-point. - Enabling more generic forms of unsizing for no-`std` smart pointers. Using the stable APIs only few concrete cases are available. One can unsize arrays to `[T]` by `ptr::slice_from_raw_parts` but unsizing a custom smart pointer to, e.g., `dyn Iterator`, `dyn Future`, `dyn Debug`, can't easily be done generically. Exposing `with_metadata_of` would allow smart pointers to offer their own `unsafe` escape hatch with similar parameters where the caller provides the unsized metadata. This is particularly interesting for embedded where `dyn`-trait usage can drastically reduce code size.

SimonSapin added 2 commits January 21, 2018 09:48

NonNull ended up landing in 1.25

3f55794

Move Debug for NonNull impl closer to other trait impls

ad37e3f

rust-highfive assigned KodrAus Jan 21, 2018

eddyb reviewed Jan 21, 2018

View reviewed changes

Implement Eq, PartialEq, Ord, PartialOrd, and Hash for NonNull<_>

6461c9b

SimonSapin force-pushed the nonnull branch 2 times, most recently from 3e1a159 to f229511 Compare January 21, 2018 09:33

SimonSapin mentioned this pull request Jan 21, 2018

Tracking issue for NonZero/Unique/Shared stabilization #27730

Closed

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 21, 2018

dtolnay reviewed Jan 21, 2018

View reviewed changes

SimonSapin force-pushed the nonnull branch from f229511 to bb4119f Compare January 21, 2018 19:53

dtolnay approved these changes Jan 22, 2018

View reviewed changes

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jan 22, 2018

Add an unstable cast() -> NonNull method to NonNull<T>.

b8ffc8a

This is less verbose than going through raw pointers to cast with `as`.

SimonSapin force-pushed the nonnull branch from bb4119f to b8ffc8a Compare January 22, 2018 08:22

SimonSapin mentioned this pull request Jan 22, 2018

Tracking issue for std::ptr::NonNull::cast #47653

Closed

kennytm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 31, 2018

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 6, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 6, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 7, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2018

Manishearth mentioned this pull request Feb 7, 2018

Rollup of 10 pull requests #48053

Merged

bors added a commit that referenced this pull request Feb 7, 2018

Auto merge of #48053 - Manishearth:rollup, r=Manishearth

29c8276

Rollup of 10 pull requests - Successful merges: #47613, #47631, #47810, #47883, #47922, #47944, #48014, #48018, #48020, #48028 - Failed merges:

bors merged commit b8ffc8a into rust-lang:master Feb 7, 2018

SimonSapin deleted the nonnull branch February 16, 2018 10:24

crlf0710 mentioned this pull request Mar 11, 2018

Improve documentation of std::ptr::NonNull #48929

Closed

HeroicKatora mentioned this pull request Mar 23, 2022

Refactor set_ptr_value as with_metadata_of #95249

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some APIs to ptr::NonNull and fix `since` attributes #47631

Add some APIs to ptr::NonNull and fix `since` attributes #47631

SimonSapin commented Jan 21, 2018 •

edited

Loading

rust-highfive commented Jan 21, 2018

eddyb Jan 21, 2018

SimonSapin Jan 21, 2018

SimonSapin Jan 21, 2018

eddyb Jan 21, 2018

KodrAus commented Jan 21, 2018

SimonSapin commented Jan 21, 2018

dtolnay Jan 21, 2018

eddyb Jan 21, 2018 •

edited

Loading

SimonSapin Jan 21, 2018

SimonSapin commented Jan 21, 2018

dtolnay left a comment

KodrAus commented Jan 22, 2018

KodrAus commented Jan 22, 2018

rfcbot commented Jan 22, 2018 •

edited by BurntSushi

Loading

SimonSapin commented Jan 22, 2018

kennytm commented Jan 31, 2018

pietroalbini commented Feb 5, 2018

rfcbot commented Feb 6, 2018

rfcbot commented Feb 6, 2018

alexcrichton commented Feb 6, 2018

bors commented Feb 6, 2018

bors commented Feb 7, 2018

bors commented Feb 7, 2018

kennytm commented Feb 7, 2018

Add some APIs to ptr::NonNull and fix since attributes #47631

Add some APIs to ptr::NonNull and fix since attributes #47631

Conversation

SimonSapin commented Jan 21, 2018 • edited Loading

rust-highfive commented Jan 21, 2018

eddyb Jan 21, 2018

Choose a reason for hiding this comment

SimonSapin Jan 21, 2018

Choose a reason for hiding this comment

SimonSapin Jan 21, 2018

Choose a reason for hiding this comment

eddyb Jan 21, 2018

Choose a reason for hiding this comment

KodrAus commented Jan 21, 2018

SimonSapin commented Jan 21, 2018

dtolnay Jan 21, 2018

Choose a reason for hiding this comment

eddyb Jan 21, 2018 • edited Loading

Choose a reason for hiding this comment

SimonSapin Jan 21, 2018

Choose a reason for hiding this comment

SimonSapin commented Jan 21, 2018

dtolnay left a comment

Choose a reason for hiding this comment

KodrAus commented Jan 22, 2018

KodrAus commented Jan 22, 2018

rfcbot commented Jan 22, 2018 • edited by BurntSushi Loading

SimonSapin commented Jan 22, 2018

kennytm commented Jan 31, 2018

pietroalbini commented Feb 5, 2018

rfcbot commented Feb 6, 2018

rfcbot commented Feb 6, 2018

alexcrichton commented Feb 6, 2018

bors commented Feb 6, 2018

bors commented Feb 7, 2018

bors commented Feb 7, 2018

kennytm commented Feb 7, 2018

Add some APIs to ptr::NonNull and fix `since` attributes #47631

Add some APIs to ptr::NonNull and fix `since` attributes #47631

SimonSapin commented Jan 21, 2018 •

edited

Loading

eddyb Jan 21, 2018 •

edited

Loading

rfcbot commented Jan 22, 2018 •

edited by BurntSushi

Loading