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

Jump to bottom

Add as_uninit-like methods to pointer types and unify documentation of as_ref methods #75392

Merged
merged 1 commit into from
Aug 18, 2020
Merged

Add as_uninit-like methods to pointer types and unify documentation of as_ref methods #75392

merged 1 commit into from
Aug 18, 2020

Conversation

TimDiekmann
Copy link
Member

@TimDiekmann TimDiekmann commented Aug 11, 2020 •
edited
Loading

This adds a convenient method to retrieve a &(mut) [MaybeUninit<T>] from slice pointers (*const [T], *mut [T], NonNull<[T]>). See also rust-lang/wg-allocators#66 (comment).

I'll add a tracking issue as soon as it's reviewed and CI passed.
Tracking Issue: #75402

r? @RalfJung

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 11, 2020
@RalfJung
Copy link
Member

RalfJung commented Aug 11, 2020 •
edited
Loading

Is there any particular reason why you added this for slices, but not for general pointers? It seems just as useful for *const T/*mut T/NonNull<T>.

@TimDiekmann
Copy link
Member Author

TimDiekmann commented Aug 11, 2020 •
edited
Loading

That would would result in MaybeUninit<[T]> rather than [MaybeUninit<T>].

@TimDiekmann
Copy link
Member Author

Maybe we could also add as_uninit for non-slices?

@RalfJung
Copy link
Member

That would would result in MaybeUninit<[T]> rather than [MaybeUninit].

We probably want a method that converts between those two as well, but I don't know a good way to add such a method. :/

Maybe we could also add as_uninit for non-slices?

That's what I meant -- something like as_uninit/as_uninit_ref or so.

@TimDiekmann
Copy link
Member Author

We probably want a method that converts between those two as well, but I don't know a good way to add such a method. :/

Me neither, we probably want a method converting in both direction with a decent name...

That's what I meant -- something like as_uninit/as_uninit_ref or so.

I'd name them as_uninit_ref to make them analogue to as_ref. Do we want another pull request for this or should I just add this here, gated behind as_uninit_ref? Should I add both to the same tracking issue?

@RalfJung
Copy link
Member

I'd make them all one PR and one feature.

@TimDiekmann
Copy link
Member Author

Currently we have NonNull::as_ref and NonNull::as_mut taking &(mut) self (both stabilized - sadly). Shall I also alter as_uninit_slice to take &self and returning &[MaybeUninit] and add as_uninit_slice_mut? Since NonNull implements Copy I don't like passing it as a reference, as you will almost never have a reference to NonNull, especially not a &mut NonNull.

This btw. also applies to as_mut_ptr which converts a slice to a non-slice pointer. On nightly there is MaybeUninit::first_ptr(_mut) which maybe could also be used on slice pointers instead. With this naming we could keep a single naming scheme.

@RalfJung
Copy link
Member

Shall I also alter as_uninit_slice to take &self and returning &[MaybeUninit] and add as_uninit_slice_mut?

I can do technical reviews but this is a question for @rust-lang/libs that I cannot help with, I am afraid.

@TimDiekmann
Copy link
Member Author

I'll first implement the methods as they were already implemented with the pointer types. Since some methods are already stable, I think that a unified schema is the most important thing.

@TimDiekmann
Copy link
Member Author

I just noticed, that pointer::as_ref returns Option<&T>.

@TimDiekmann TimDiekmann marked this pull request as draft August 11, 2020 11:18
@TimDiekmann TimDiekmann changed the title Add as_uninit_slice to slice pointer types Add as_uninit-like methods to pointer types Aug 11, 2020
@TimDiekmann TimDiekmann marked this pull request as ready for review August 11, 2020 13:00
@TimDiekmann TimDiekmann mentioned this pull request Aug 11, 2020
Open
3 tasks
@TimDiekmann
Copy link
Member Author

Is there any particular reason why you added this for slices, but not for general pointers? It seems just as useful for *const T/*mut T/NonNull<T>.

One thing to note: MaybeUninit<[T]> isn't even possible right now, as MaybeUninit<T> requires T to be sized.
Noticed that when I implemented as_uninit_ref/mut :D

@RalfJung
Copy link
Member

Ah good point, and we are unlikely to be able to relax that sized bound.

RalfJung
RalfJung reviewed Aug 12, 2020
View reviewed changes
library/core/src/ptr/const_ptr.rs Outdated Show resolved Hide resolved
library/core/src/ptr/const_ptr.rs Outdated Show resolved Hide resolved
library/core/src/ptr/mut_ptr.rs Outdated Show resolved Hide resolved
library/core/src/ptr/mut_ptr.rs Outdated Show resolved Hide resolved
library/core/src/ptr/mut_ptr.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
@TimDiekmann
Copy link
Member Author

Those intra-doc links drive me crazy :D
CI should pass now.

RalfJung
RalfJung reviewed Aug 13, 2020
View reviewed changes
library/core/src/ptr/const_ptr.rs Outdated Show resolved Hide resolved
library/core/src/ptr/const_ptr.rs Outdated Show resolved Hide resolved
library/core/src/ptr/const_ptr.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

In terms of text this is all fine (assuming it's the same text for correspond methods, which I did not check in detail everywhere). I just have some more typographic nits.

@TimDiekmann
Copy link
Member Author

TimDiekmann commented Aug 16, 2020 •
edited
Loading

Thank you!
I'll double-check every text and it's method again and will push it later this day, probably tonight.

@TimDiekmann TimDiekmann changed the title Add as_uninit-like methods to pointer types Add as_uninit-like methods to pointer types and unify documentation of as_ref methods Aug 16, 2020
@RalfJung
Copy link
Member

All right, I think this is ready. :) Could you squash the commits?

@TimDiekmann
Add as_uninit-like methods to pointer types and unify documentation…
93e074b 
… of `as_ref` methods

Fix example in `NonNull::as_uninit_slice`


Rename feature gate to "ptr_as_uninit"


Make methods more consistent with already stable methods


Make `pointer::as_uninit_slice` return an `Option`


Fix placement for `// SAFETY` section


Add `as_uninit_ref` and `as_uninit_mut` to pointers


Fix doctest


Update tracking issue


Fix doc links


Apply suggestions from review


Make wording about counterparts consistent


Fix doc links


Improve documentation

Fix doc-tests


Fix doc links ... again


Apply suggestions from review


Apply suggestions from Review


Apply suggestion from review to all affected files


Add missing words in safety sections in `as_uninit_slice_mut`


Fix safety-comment in `NonNull::as_uninit_slice_mut`
@TimDiekmann
Copy link
Member Author

TimDiekmann commented Aug 17, 2020 •
edited
Loading

Done (in case GH does not send mails on force-pushes).

@RalfJung
Copy link
Member

RalfJung commented Aug 17, 2020 •
edited
Loading

Done (in case GH does not send mails on force-pushes).

(it doesn't, except sometimes when it does... their support told me it only sends them when the parent of the new HEAD was already in the branch, aka when you just amended the last commit.)

Great, and thanks for enduring by never-ending stream of editorial comments. :)
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 17, 2020

📌 Commit 93e074b has been approved by RalfJung

@bors 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 Aug 17, 2020
@bors

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@TimDiekmann
Copy link
Member Author

Great, and thanks for enduring by never-ending stream of editorial comments. :)

I think the effort was worth it :)

@JohnTitor JohnTitor mentioned this pull request Aug 18, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 18, 2020
@bors
Auto merge of rust-lang#75653 - JohnTitor:rollup-0ejtdfo, r=JohnTitor
d7dcae0 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#75389 (attempt to improve span_label docs)
 - rust-lang#75392 (Add `as_uninit`-like methods to pointer types and unify documentation of `as_ref` methods)
 - rust-lang#75464 (Move to intra doc links for ascii.rs and panic.rs)
 - rust-lang#75578 (Allowing raw ptr dereference in const fn)
 - rust-lang#75613 (Add explanation for `&mut self` method call when expecting `-> Self`)
 - rust-lang#75626 (Clean up E0754 explanation)
 - rust-lang#75629 (Use intra-doc links in `std::env`, `std::alloc` and `std::error`)
 - rust-lang#75634 (Mark x86_64-linux-kernel as *)

Failed merges:

r? @ghost
@bors bors merged commit 5498367 into rust-lang:master Aug 18, 2020
@TimDiekmann TimDiekmann deleted the non-null-uninit-slice branch November 18, 2020 16:59
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@RalfJung RalfJung

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.47.0
Development

Successfully merging this pull request may close these issues.
5 participants
@TimDiekmann @RalfJung @bors @cuviper @rust-highfive