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.

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

Implement reserve_len and reserve_len_exact for VecMap #20143

Merged
merged 1 commit into from
Dec 27, 2014

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
Collaborator

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
///
/// The collection may reserve more space to avoid frequent reallocations.
///
/// # Examples
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