clarify comment in RawVec::into_box

[RalfJung]

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]

r? @shepmaster

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

[RalfJung]

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

[Dylan-DPC-zz]

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

@bors r+ rollup

[bors]

📌 Commit bbab6db36f738b26fec914b4ef3872ea8c6854ee has been approved by Dylan-DPC

[TimDiekmann]

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]

@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]

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]

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]

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?

[TimDiekmann]

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.

[TimDiekmann]

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.

[RalfJung]

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.

[RalfJung]

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?

[TimDiekmann]

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.

[RalfJung]

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

[TimDiekmann]

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]

@bors r-

[RalfJung]

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?

[TimDiekmann]

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]

@bors r=Dylan-DPC,TimDiekmann

[bors]

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