Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

csouth3
Copy link
Contributor

@csouth3 csouth3 commented Dec 22, 2014

Implement reserve_len and reserve_len_exact for VecMap in accordance with rust-lang/rfcs#509.

@rust-highfive
Copy link
Contributor

r? @nikomatsakis

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

@csouth3
Copy link
Contributor Author

csouth3 commented Dec 22, 2014

r? @gankro

@rust-highfive rust-highfive assigned Gankra and unassigned nikomatsakis Dec 22, 2014
Copy link
Contributor

Choose a reason for hiding this comment

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

I settled on the following wording in BitvSet:

/// Reserves capacity for the given `BitvSet` to contain `len` distinct elements. In the case
/// of `BitvSet` this means reallocations will not occur as long as all inserted elements
/// are less than `len`.
///
/// The collection may reserve more space to avoid frequent reallocations.

I worded this to clarify "why len" and future-proof for a possible future where Vec and RingBuf get it. Willing to bikeshed on it, 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.

So then would the correct wording for VecMap be to change the first sentence to
"Reserves capacity for the given VecMap to contain len distinct elements..." and then keep the rest of what I had, or should the second sentence be changed to mirror BitvSet too? Can't quite tell what would make sense

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah s/BitvSet/VecMap; they have basically the same semantics (the high-level documentation even suggests that BitvSet is basically VecSet).

@Gankra
Copy link
Contributor

Gankra commented Dec 22, 2014

Only issue is the method description bikeshed.

@Gankra
Copy link
Contributor

Gankra commented Dec 22, 2014

Also for posterity, exact:

/// Reserves the minimum capacity for the given `BitvSet` to contain `len` distinct elements.
/// In the case of `BitvSet` this means reallocations will not occur as long as all inserted
/// elements are less than `len`.
///
/// Note that the allocator may give the collection more space than it requests. Therefore
/// capacity can not be relied upon to be precisely minimal. Prefer `reserve_len` if future
/// insertions are expected.

@csouth3
Copy link
Contributor Author

csouth3 commented Dec 22, 2014

Method descriptions fixed.

@csouth3
Copy link
Contributor Author

csouth3 commented Dec 22, 2014

Alright, take 3!

bors added a commit that referenced this pull request Dec 27, 2014
Implement `reserve_len` and `reserve_len_exact` for `VecMap` in accordance with rust-lang/rfcs#509.
@bors bors merged commit 868acdf into rust-lang:master Dec 27, 2014
@csouth3 csouth3 deleted the vecmap-reserve branch December 27, 2014 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.