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

sgrif · 2017-02-09T19:01:40Z

I spent a good chunk of time tracking down a buffer overrun bug that
resulted from me mistakenly thinking that reserve was based on the
current capacity not the current length. It would be helpful if this
were called out explicitly in the docs.

I spent a good chunk of time tracking down a buffer overrun bug that resulted from me mistakenly thinking that `reserve` was based on the current capacity not the current length. It would be helpful if this were called out explicitly in the docs.

keeperofdakeys · 2017-02-09T21:04:30Z

While you're here, I wonder if you shouldn't update the truncate and clear documentation, mentioning that they explicitly don't touch capacity. The last time I used them, I had to verify their functionality from the code :/.

alexcrichton · 2017-02-10T02:16:17Z

@bors: r+

Thanks @sgrif! We should always be able to follow up with @keeperofdakeys's suggestion in a further PR

bors · 2017-02-10T02:16:18Z

📌 Commit b3937ea has been approved by alexcrichton

alexcrichton · 2017-02-10T02:16:21Z

@bors: rollup

bors · 2017-02-10T09:47:45Z

⌛ Testing commit b3937ea with merge c20ce8e...

bors · 2017-02-10T09:51:03Z

💔 Test failed - status-appveyor

alexcrichton · 2017-02-10T15:01:42Z

@bors: retry * github network failure

…

On Fri, Feb 10, 2017 at 1:51 AM, bors ***@***.***> wrote: 💔 Test failed - status-appveyor <https://ci.appveyor.com/project/rust-lang/rust/build/1.0.1886> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#39701 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95J0PIGai1Dyu6ZUlrrTLNUev6ENUks5rbDMJgaJpZM4L8h-Q> .

codyps · 2017-02-10T15:49:50Z

src/libcollections/vec.rs

-    /// 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`.


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?

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.

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.

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.

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.

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

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.

@sgrif perhaps:

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

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.

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

…ichton Explicitly mention that `Vec::reserve` is based on len not capacity I spent a good chunk of time tracking down a buffer overrun bug that resulted from me mistakenly thinking that `reserve` was based on the current capacity not the current length. It would be helpful if this were called out explicitly in the docs.

nagisa · 2017-02-10T16:47:58Z

src/libcollections/vec.rs

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


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

I personally prefer "greater than or equal to".

frewsxcv · 2017-02-10T17:03:57Z

@bors r-

alexcrichton · 2017-02-11T00:31:26Z

@bors: r+

bors · 2017-02-11T00:31:27Z

📌 Commit b3937ea has been approved by alexcrichton

…ichton Explicitly mention that `Vec::reserve` is based on len not capacity I spent a good chunk of time tracking down a buffer overrun bug that resulted from me mistakenly thinking that `reserve` was based on the current capacity not the current length. It would be helpful if this were called out explicitly in the docs.

Rollup of 9 pull requests - Successful merges: #39174, #39660, #39676, #39692, #39701, #39710, #39721, #39724, #39725 - Failed merges:

codyps reviewed Feb 10, 2017

View reviewed changes

frewsxcv mentioned this pull request Feb 10, 2017

Rollup of 5 pull requests #39718

Closed

nagisa reviewed Feb 10, 2017

View reviewed changes

frewsxcv mentioned this pull request Feb 11, 2017

Rollup of 8 pull requests #39735

Closed

frewsxcv mentioned this pull request Feb 11, 2017

Rollup of 9 pull requests #39736

Merged

bors added a commit that referenced this pull request Feb 11, 2017

Auto merge of #39736 - frewsxcv:rollup, r=frewsxcv

bae454e

Rollup of 9 pull requests - Successful merges: #39174, #39660, #39676, #39692, #39701, #39710, #39721, #39724, #39725 - Failed merges:

bors merged commit b3937ea into rust-lang:master Feb 11, 2017

sgrif deleted the sg-vec-reserve-docs branch February 11, 2017 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sgrif commented Feb 9, 2017

keeperofdakeys commented Feb 9, 2017

alexcrichton commented Feb 10, 2017

bors commented Feb 10, 2017

alexcrichton commented Feb 10, 2017

bors commented Feb 10, 2017

bors commented Feb 10, 2017

alexcrichton commented Feb 10, 2017 via email

codyps Feb 10, 2017

frewsxcv Feb 10, 2017

codyps Feb 10, 2017 •

edited

Loading

frewsxcv Feb 10, 2017

nagisa Feb 10, 2017

codyps Feb 10, 2017 •

edited

Loading

sgrif Feb 10, 2017

alexcrichton Feb 11, 2017

ollie27 Feb 11, 2017

alexcrichton Feb 11, 2017

nagisa Feb 10, 2017

frewsxcv Feb 10, 2017

frewsxcv commented Feb 10, 2017

alexcrichton commented Feb 11, 2017

bors commented Feb 11, 2017

	/// 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.

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

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

Conversation

sgrif commented Feb 9, 2017

keeperofdakeys commented Feb 9, 2017

alexcrichton commented Feb 10, 2017

bors commented Feb 10, 2017

alexcrichton commented Feb 10, 2017

bors commented Feb 10, 2017

bors commented Feb 10, 2017

alexcrichton commented Feb 10, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyps Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyps Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frewsxcv commented Feb 10, 2017

alexcrichton commented Feb 11, 2017

bors commented Feb 11, 2017

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

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

codyps Feb 10, 2017 •

edited

Loading

codyps Feb 10, 2017 •

edited

Loading