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

Explicitly mention that Vec::reserve is based on len not capacity #39701

Merged
merged 1 commit into from
Feb 11, 2017
Merged

Explicitly mention that Vec::reserve is based on len not capacity #39701

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 6 additions & 3 deletions src/libcollections/vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,9 @@ impl<T> Vec<T> {

/// Reserves capacity for at least `additional` more elements to be inserted
/// in the given `Vec<T>`. The collection may reserve more space to avoid
/// frequent reallocations.
/// frequent reallocations. After calling `reserve`, capacity will be
/// greater than or equal to `self.len() + additional`. Does nothing if
Copy link
Member

Choose a reason for hiding this comment

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

“not less than” is a better way to phrase “greater than or equal to”

Copy link
Member

Choose a reason for hiding this comment

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

I personally prefer "greater than or equal to".

/// capacity is already sufficient.
///
/// # Panics
///
Expand All @@ -456,8 +458,9 @@ impl<T> Vec<T> {
}

/// Reserves the minimum capacity for exactly `additional` more elements to
/// be inserted in the given `Vec<T>`. Does nothing if the capacity is already
/// sufficient.
/// be inserted in the given `Vec<T>`. After calling `reserve_exact`,
/// capacity will be greater than or equal to `self.len() + additional`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be "equal to" rather than "greater than or equal to" given that this "reserves the minimum capacity for [an exact number of additional elements]"?

Or is there a reason to leave the bound open?

Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth, here's a relevant comment in the underlying implementation:

rust/src/liballoc/raw_vec.rs

Lines 275 to 280 in b3937ea

/// Ensures that the buffer contains at least enough space to hold
/// `used_cap + needed_extra_cap` elements. If it doesn't already,
/// will reallocate the minimum possible amount of memory necessary.
/// Generally this will be exactly the amount of memory necessary,
/// but in principle the allocator is free to give back more than
/// we asked for.

Copy link
Contributor

@codyps codyps Feb 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, there may be more backing memory. But the Vec::capacity() (which is what I'm presuming capacity in the comment refers to here) won't know about it.

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 you're right. Don't want to mislead people with "greater than or equal to" since it doesn't seem accurate in this case.

Copy link
Member

Choose a reason for hiding this comment

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

The assertion in example below has >=. The implementation calculates the new capacity and just sets it to self.cap (so ==). The point of the method is to have it be == IIRC.

Copy link
Contributor

@codyps codyps Feb 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@sgrif I had presumed that precisely minimal in that context implied capacity can not be relied upon to be precisely minimal in describing the backing allocation size, but you're correct that it isn't entirely clear. Is there a desire to allow capacity to (at some point) more accurately expand to the size of the backing memory returned from the allocator? Could allow eliding calling into the allocator in some cases.

@alexcrichton I had taken the Does nothing if the capacity is already sufficient. line which immediately follows to create an exception to that. The added sentence could be reworded to make that exception explicit, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to change if y'all think it's necessary. I went with the most conservative statement (and the one that matches the code example). I think if this statement changes, the assertion in the example should change to use == as well, however.

Copy link
Member

Choose a reason for hiding this comment

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

@sgrif perhaps:

Does nothing if the capacity is already sufficient, or otherwise ensures the capacity is equal to self.len() + additional upon returning

Copy link
Member

Choose a reason for hiding this comment

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

otherwise ensures the capacity is equal to self.len() + additional upon returning

I'm pretty sure we don't guarantee that at the moment. Do we want to start guaranteeing that now? Other collections have a reserve_exact method which acts the same so any changes should be made to all of them, not just Vec.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, in that case let's just merge as-is.

/// Does nothing if the capacity is already sufficient.
///
/// Note that the allocator may give the collection more space than it
/// requests. Therefore capacity can not be relied upon to be precisely
Expand Down