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

Add Vec visualization to understand capacity #76066

Closed
wants to merge 11 commits into from
Closed

Add Vec visualization to understand capacity #76066

Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9c1415e
Add Vec visualization to understand capacity
pickfire Aug 29, 2020
ea337c6
Center align ptr and len box vec doc
pickfire Aug 29, 2020
f2bd46b
Link uninitialized memory to MaybeUninit
pickfire Aug 30, 2020
53af7b8
Vec visualization separate description
pickfire Aug 30, 2020
a919516
Link Vec visualization to core::mem::MaybeUninit
pickfire Aug 31, 2020
55ad7f2
Vec visualization mention unstable ABI
pickfire Sep 1, 2020
44e9b7f
Use explicit details to relate to the diagram
pickfire Oct 15, 2020
6900744
Make vec note for visualization more explicit
pickfire Oct 15, 2020
79918c1
Use alphabetical example in vec visualization
pickfire Oct 27, 2020
62ba5b1
Fix vec doc merge conflict
pickfire Dec 1, 2020
702662e
Merge branch 'master' into patch-15
pickfire Dec 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions library/alloc/src/vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,24 @@ use crate::raw_vec::RawVec;
/// can be slow. For this reason, it is recommended to use [`Vec::with_capacity`]
/// whenever possible to specify how big the vector is expected to get.
///
/// A vector containing the elements `'a'` and `'b'` with capacity 4 can be visualized as:
/// ```text
/// Stack ptr capacity len
Copy link
Member

Choose a reason for hiding this comment

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

"Stack" is not correct. Those three fields don't have to live on the stack, e.g. Box<Vec<T>>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, Box<Vec<T>> is not good, I believe we already mentioned this somewhere in the book, Vec itself is already sort of a smart pointer, no point wrapping box around. But in this case it is talking about Vec<T> which it is on the stack. I think we don't need to add note on this since this is a Box thing, if we want to add it should be in Box.

Copy link
Member

Choose a reason for hiding this comment

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

A vec can also live inside some other struct that lives on the heap, e.g. an Arc. Maybe just call it "struct"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But doesn't struct and heap when used together feels weird?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just remove "Stack". We simply don't know where those three values live. We just know that the lower box lives on the heap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about Stack/Heap? Since it could be either stack or heap.

Copy link
Member

Choose a reason for hiding this comment

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

Try 'Layout' instead, maybe?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like "The Vec object itself" and "The data on the heap".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Try 'Layout' instead, maybe?

Layout isn't clear where it will be.

Maybe something like "The Vec object itself" and "The data on the heap".

Too long. Also having "object" word itself can confuse a lot of people. "The data on the heap" is incorrect, since it could be in the stack too.

/// +--------+--------+--------+
/// | 0x0123 | 4 | 2 |
Copy link
Member

Choose a reason for hiding this comment

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

into_raw_parts and from_raw_parts use (ptr, length, capacity), not (ptr, capacity, length). It might be good to use the same order here.

Copy link
Member

Choose a reason for hiding this comment

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

This is intentionally different, see #76066 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

@jyn514 That comment suggests to use a different layout than the real layout. The 'real' layout is (RawVec, len), which is effectively a (ptr, cap, len). Changing it to (ptr, len, cap) as I suggest does make it different from the actual layout, just like the8472 asked for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird, I didn't notice this, I thought I used len followed by capacity.

/// +--------+--------+--------+
LukasKalbertodt marked this conversation as resolved.
Show resolved Hide resolved
/// |
/// v
/// Heap +--------+--------+--------+--------+
/// | 1 | 2 | uninit | uninit |
pickfire marked this conversation as resolved.
Show resolved Hide resolved
/// +--------+--------+--------+--------+
pickfire marked this conversation as resolved.
Show resolved Hide resolved
/// ```
///
/// - **uninit** represents memory that is not initialized, see [`MaybeUninit`].
/// - Note: the ABI is not stable and `Vec` makes no guarantees about its memory
/// layout (including the order of fields). See [the section about
/// guarantees](#guarantees).
Comment on lines +218 to +220
Copy link
Member

Choose a reason for hiding this comment

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

The layout of the part on the heap is pretty strict. Let's make clear it's only the Vec object itself (the (ptr, size, cap) part) that's unspecified. A mention of into_raw_parts/from_raw_parts and/or .as_ptr()/.len()/.capacity() would also be nice here, as those are the correct way of interacting with these fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe you can suggest a change here?

///
/// # Guarantees
///
/// Due to its incredibly fundamental nature, `Vec` makes a lot of guarantees
Expand Down Expand Up @@ -287,6 +305,7 @@ use crate::raw_vec::RawVec;
/// [`insert`]: Vec::insert
/// [`reserve`]: Vec::reserve
/// [owned slice]: Box
/// [`MaybeUninit`]: core::mem::MaybeUninit
pickfire marked this conversation as resolved.
Show resolved Hide resolved
#[stable(feature = "rust1", since = "1.0.0")]
#[cfg_attr(not(test), rustc_diagnostic_item = "vec_type")]
pub struct Vec<T> {
Expand Down