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.

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

Improve MaybeUninit::get_{ref,mut} documentation #65948

Merged
merged 4 commits into from
Nov 5, 2019

Conversation

danielhenrymantilla
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla commented Oct 29, 2019

As mentioned in #63568 (comment), MaybeUninit's get_{ref,mut} documentation is lacking, so this PR attempts to fix that.

That being said, and as @RalfJung mentions in that thread,

In particular, we should clarify that all the UB rules for these methods equally apply when calling the raw ptr methods and creating a reference manually.

these other docs also need to be improved, which I can do in this PR (hence the [WIP]).

Finally, since all these documentations are related to clearly establishing when dealing with uninitialized memory which patterns are known to be sound and which patterns are currently UB (that is, until, if ever, the rules around references to unintialized integers get relaxed, this documentation will treat them as UB, and advise against such patterns (e.g., it is not possible to use uninitialized buffers with the Read API)), I think that adding even more examples to the main documentation of MaybeUninit inherent definition wouldn't hurt either.


@rust-highfive
Copy link
Collaborator

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 29, 2019
@danielhenrymantilla
Copy link
Contributor Author

r? @RalfJung cc @SimonSapin @Centril

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

Great work! These are very good examples. I just found some nits.

@Centril do you also want to take a look?

src/libcore/mem/maybe_uninit.rs Outdated Show resolved Hide resolved
src/libcore/mem/maybe_uninit.rs Outdated Show resolved Hide resolved
src/libcore/mem/maybe_uninit.rs Outdated Show resolved Hide resolved
src/libcore/mem/maybe_uninit.rs Outdated Show resolved Hide resolved
src/libcore/mem/maybe_uninit.rs Outdated Show resolved Hide resolved
src/libcore/mem/maybe_uninit.rs Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Nov 2, 2019

@RalfJung Thanks for the ping. I trust your review. :)

@danielhenrymantilla
Copy link
Contributor Author

Okay, I have fixed "the nits", so I think that regarding get_ref and get_mut documentation, we should be good. Should we take advantage of this PR and potentially enhance the main struct's documentation as well as .as_ptr() / .as_mut_ptr()'s ?

@danielhenrymantilla danielhenrymantilla changed the title [WIP] Improve MaybeUninit::get_{ref,mut} documentation Improve MaybeUninit::get_{ref,mut} documentation Nov 4, 2019
@RalfJung
Copy link
Member

RalfJung commented Nov 4, 2019

Should we take advantage of this PR and potentially enhance the main struct's documentation as well as .as_ptr() / .as_mut_ptr()'s ?

I prefer smaller PRs, that's much easier to review.

@bors delegate+

r=me with the last nit fixed (this means you can do bors r=RalfJung then)

@bors
Copy link
Contributor

bors commented Nov 4, 2019

✌️ @danielhenrymantilla can now approve this pull request

Co-Authored-By: Ralf Jung <post@ralfj.de>
@danielhenrymantilla
Copy link
Contributor Author

@bors r=RalfJung

@bors
Copy link
Contributor

bors commented Nov 4, 2019

📌 Commit 67f2200 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 Nov 4, 2019
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 5, 2019
…t_ref_mut, r=RalfJung

Improve MaybeUninit::get_{ref,mut} documentation

As mentioned in rust-lang#63568 (comment), `MaybeUninit`'s `get_{ref,mut}` documentation is lacking, so this PR attempts to fix that.

That being said, and as @RalfJung mentions in that thread,

> In particular, we should clarify that all the UB rules for these methods equally apply when calling the raw ptr methods and creating a reference manually.

these other docs also need to be improved, which I can do in this PR ~~(hence the `[WIP]`)~~.

Finally, since all these documentations are related to clearly establishing when dealing with uninitialized memory which patterns are known to be sound and which patterns are currently UB (that is, until, if ever, the rules around references to unintialized integers get relaxed, this documentation will treat them as UB, and advise against such patterns (_e.g._, it is not possible to use uninitialized buffers with the `Read` API)), I think that adding even more examples to the main documentation of `MaybeUninit` inherent definition wouldn't hurt either.

___

  - [Rendered](http://dreamy-ritchie-99d637.netlify.com/core/mem/union.maybeuninit#method.get_ref)
bors added a commit that referenced this pull request Nov 5, 2019
Rollup of 8 pull requests

Successful merges:

 - #65948 (Improve MaybeUninit::get_{ref,mut} documentation)
 - #65953 (Allow specifying LLVM's MCTargetOptions::ABIName in target specification files)
 - #66012 (De-querify `trivial_dropck_outlives`.)
 - #66025 (`Span` cannot represent `span.hi < span.lo`)
 - #66047 (Don't double-count `simd_shuffle` promotion candidates)
 - #66053 (when Miri tests are not passing, do not add Miri component)
 - #66082 (clean highlightSourceLines code)
 - #66091 (Implemented the home_dir for VxWorks)

Failed merges:

r? @ghost
@bors bors merged commit 67f2200 into rust-lang:master Nov 5, 2019
@danielhenrymantilla danielhenrymantilla deleted the doc/maybe_uninit_ref_mut branch November 6, 2019 21:26
@pr2502
Copy link
Contributor

pr2502 commented Feb 22, 2020

I'm sorry if this is inappropriate time or place to write this, but this is such a minor thing I hope it's better than filing an issue?

The code snippet in get_mut() documentation is incorrectly asserting the array being sorted:

assert!(
    buf.chunks(2).all(|chunk| chunk[0] <= chunk[1]),
    "buffer is sorted",
);

It should use the slice::windows method instead of slice::chunks to actually check if all elements are sorted, this test would let an array like [3, 4, 1, 2] pass as sorted.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 24, 2020
Suggested by @ametisf in rust-lang#65948 (comment)

Co-Authored-By: Frantisek Fladung <fladufra@fel.cvut.cz>
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 24, 2020
…cs_replace_chunk_with_windows, r=Dylan-DPC

Fix minor error in `MaybeUninit::get_mut()` doc example

In the `MaybeUninit::get_mut()` example I wanted to assert that the slice was sorted and mistakenly used `.chunks(2)` rather than `.windows(2)` to assert it, as @ametisf pointed out in rust-lang#65948 (comment) .

This fixes it.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 24, 2020
…cs_replace_chunk_with_windows, r=Dylan-DPC

Fix minor error in `MaybeUninit::get_mut()` doc example

In the `MaybeUninit::get_mut()` example I wanted to assert that the slice was sorted and mistakenly used `.chunks(2)` rather than `.windows(2)` to assert it, as @ametisf pointed out in rust-lang#65948 (comment) .

This fixes it.
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Feb 24, 2020
…cs_replace_chunk_with_windows, r=Dylan-DPC

Fix minor error in `MaybeUninit::get_mut()` doc example

In the `MaybeUninit::get_mut()` example I wanted to assert that the slice was sorted and mistakenly used `.chunks(2)` rather than `.windows(2)` to assert it, as @ametisf pointed out in rust-lang#65948 (comment) .

This fixes it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants