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

clarify comment in RawVec::into_box #70776

Merged
merged 1 commit into from
Apr 5, 2020
Merged

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 4, 2020

On first reading I almost thought "len <= cap" would be all that there is to check here. Expand the comment to clarify that that is not the case.

@rust-highfive
Copy link
Collaborator

r? @shepmaster

(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 Apr 4, 2020
@RalfJung
Copy link
Member Author

RalfJung commented Apr 4, 2020

That's a weird PR CI failure?

Build completed successfully in 0:00:29
+ /scripts/validate-toolstate.sh
Cloning into 'rust-toolstate'...
Traceback (most recent call last):
  File "../../src/tools/publish_toolstate.py", line 305, in <module>
    cur_datetime
  File "../../src/tools/publish_toolstate.py", line 205, in update_latest
    maintainers = ' '.join('@'+name for name in MAINTAINERS[tool])
KeyError: u'rustc-dev-guide'

Cc @pietroalbini

@rust-highfive

This comment has been minimized.

@Dylan-DPC-zz
Copy link

Yeah the failure was due to some other issue, don't worry

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 5, 2020

📌 Commit bbab6db36f738b26fec914b4ef3872ea8c6854ee has been approved by Dylan-DPC

@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 Apr 5, 2020
@TimDiekmann
Copy link
Member

Currently, Box<[MaybeUninit<T>]> is not yet mature, so it is assumed that shrink_to_fit is called to be conservative. Later, len < capacity will probably be enough, since AllocRef::dealloc can be called with a layout that fits the pointer:

The provided layout.size() must fall in the range min ..= max, where:

  • min is the size of the layout most recently used to allocate the block, and
  • max is the latest actual size returned from alloc, grow, or shrink.

@RalfJung
Copy link
Member Author

RalfJung commented Apr 5, 2020

@TimDiekmann Assume a RawVec with capacity 1024. Knowing nothing else, why would a size of 4 be guaranteed to "fit" the pointer for this backing store? It is certainly not "the size of the layout most recently used" (that would be 1024, I assume) -- and without checking the "latest actual size returned", we cannot just assume that 4 will "fit".

In other words, I am rather confused by your statement.

@TimDiekmann
Copy link
Member

Oh, I mistakenly omitted that len must also be larger than or equal to the capacity requested, this does not require shrink_to_fit; calling RawVec::with_capacity() also ensures this, thanks for pointing it out!
Forgive me, it's Sunday and it's early 😄

@TimDiekmann
Copy link
Member

TimDiekmann commented Apr 5, 2020

Actually RawVec::with_capacity() is used for impl<T: Copy> From<&[T]> for Box<[T]>:

fn from(slice: &[T]) -> Box<[T]> {
    let len = slice.len();
    let buf = RawVec::with_capacity(len);
    unsafe {
        ptr::copy_nonoverlapping(slice.as_ptr(), buf.ptr(), len);
        buf.into_box(len).assume_init()
    }
}

It's also used for Box::new_uninit_slice:

pub fn new_uninit_slice(len: usize) -> Box<[mem::MaybeUninit<T>]> {
    unsafe { RawVec::with_capacity(len).into_box(len) }
}

@RalfJung
Copy link
Member Author

RalfJung commented Apr 5, 2020

len must also be larger than or equal to the capacity requested

Yeah that makes so much more sense. :D
So, should I add that to the text, replacing the shrink_to_fit thing?

Comment on lines 574 to 576
/// `shrink_to_fit(len)` must be called immediately prior to calling this function.
/// In particular, this implies that `len` must be smaller than or equal to `self.capacity()`
/// (but that is just a *necessary*, not a *sufficient*, condition).
Copy link
Member

@TimDiekmann TimDiekmann Apr 5, 2020

Choose a reason for hiding this comment

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

So, should I add that to the text, replacing the shrink_to_fit thing?

Yes, I think so. Maybe like this?

Suggested change
/// `shrink_to_fit(len)` must be called immediately prior to calling this function.
/// In particular, this implies that `len` must be smaller than or equal to `self.capacity()`
/// (but that is just a *necessary*, not a *sufficient*, condition).
/// * `len` must be greater than or equal to the capacity requested in the most previous call, and
/// * `len` must be less than or equal to `self.capacity()`.
///
/// Note, that the requested capacity and `self.capacity()` may not be the same, as
/// an allocator may overallocate and return a greater memory block than requested.

Copy link
Member

Choose a reason for hiding this comment

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

The thing mentioned in the note was the reason why I added a len parameter to the method by the way. It's also the reason why we don't have to special case the internal capacity field anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I'll use that and edit some more!
"may not be the same" sounds like "must not be the same", so that is a bit confusing. Also, I think "most recent call" is the more common expression.

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing mentioned in the note was the reason why I added a len parameter to the method by the way. It's also the reason why we don't have to special case the internal capacity field anymore.

So if I see it correctly, the difference between self.cap and self.capacity() only arises in case of mem::size_of::<T>() == 0. And in that case the size of the Box is zero anyway so it does not matter?

Copy link
Member

Choose a reason for hiding this comment

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

I think your current wording of your latest commit is fine.

The unsafetyness is basically because len * mem::size_of::<T>() is passed to AllocRef::dealloc when dropping the Box, this must be a valid call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but I don't understand why self.cap vs self.capacity() was important, and why it is not relevant any more?

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why self.cap vs self.capacity() was important, and why it is not relevant any more?

It was only important as calls to shrink_to_fit have set self.cap in any case, even for ZSTs. Calling into_box then returned a boxed slice with the boxed.len() == self.cap. As len is now passed explicitly to into_box, self.cap isn't used here anymore.

So if I see it correctly, the difference between self.cap and self.capacity() only arises in case of mem::size_of::() == 0. And in that case the size of the Box is zero anyway so it does not matter?

Yes, that's correct. When using RawVec<ZST>, AllocRef is called with a layout.size() == 0, and self.capacity() always return usize::MAX. Theoretically any length can be passed to into_box here. However, the safety constraint is still a little bit more conservative. For example calling RawVec::<ZST>::with_capacity(10) forbids calling into_box(5), but this would be well defined. Personally I'd keep the safety clause as mentioned above, because then ZSTs and none-ZSTs are exchangeable.

@RalfJung
Copy link
Member Author

RalfJung commented Apr 5, 2020

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 5, 2020
@bors

This comment has been minimized.

@RalfJung
Copy link
Member Author

RalfJung commented Apr 5, 2020

Currently, Box<[MaybeUninit]> is not yet mature, so it is assumed that shrink_to_fit is called to be conservative.

I don't entirely understand this part, is that a concern for the edits we are planning here?

@RalfJung RalfJung added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 5, 2020
@TimDiekmann
Copy link
Member

TimDiekmann commented Apr 5, 2020

I don't entirely understand this part, is that a concern for the edits we are planning here?

No, the edits are fine. We should just keep in mind that the convenient API (like assume_init) of Box<[MaybeUninit<T>]> is not stable yet and should be used with care. Calling shrink_to_fit to the desired length will just ensure, that the both criteria in the safety clause are fulfiilled.

@RalfJung
Copy link
Member Author

RalfJung commented Apr 5, 2020

@bors r=Dylan-DPC,TimDiekmann

@bors
Copy link
Contributor

bors commented Apr 5, 2020

📌 Commit 6cbe172 has been approved by Dylan-DPC,TimDiekmann

@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 Apr 5, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 5, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#70558 (Fix some aliasing issues in Vec)
 - rust-lang#70760 (docs: make the description of Result::map_or more clear)
 - rust-lang#70769 (Miri: remove an outdated FIXME)
 - rust-lang#70776 (clarify comment in RawVec::into_box)
 - rust-lang#70806 (fix Miri assignment sanity check)

Failed merges:

r? @ghost
@bors bors merged commit c185c4f into rust-lang:master Apr 5, 2020
@RalfJung RalfJung deleted the raw-vec branch April 5, 2020 16:29
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.

6 participants