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

Explain how Vec::with_capacity is faithful #135933

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hkBst
Copy link
Contributor

@hkBst hkBst commented Jan 23, 2025

This is a revival of #99790 building on the prose of @workingjubilee and edits of @jmaargh. Closes #99385.

@rustbot
Copy link
Collaborator

rustbot commented Jan 23, 2025

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 23, 2025
Copy link
Contributor

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Awesome! Not only is this very clear and understandable, it's also a good design that can make use of advanced allocator capabilities.

Linking #101316 for convenience.

/// and no other size (such as, for example: a size rounded up to the nearest power of 2).
/// The allocator will return an allocation that is at least as large as requested, but it may be larger.
///
/// It is guaranteed that the [Vec::capacity] method returns a value that is at least the requested capacity
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// It is guaranteed that the [Vec::capacity] method returns a value that is at least the requested capacity
/// It is guaranteed that the [`Vec::capacity`] method returns a value that is at least the requested capacity

/// It is guaranteed that the [Vec::capacity] method returns a value that is at least the requested capacity
/// and not more than the allocated capacity.
///
/// The method [Vec::shrink_to_fit] can discard excess capacity an allocator has given to a `Vec`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// The method [Vec::shrink_to_fit] can discard excess capacity an allocator has given to a `Vec`.
/// The method [`Vec::shrink_to_fit`] can discard excess capacity an allocator has given to a `Vec`.

@ibraheemdev
Copy link
Member

r? @workingjubilee because there were some open questions on the original PR.

@rustbot
Copy link
Collaborator

rustbot commented Jan 31, 2025

Could not assign reviewer from: workingjubilee.
User(s) workingjubilee are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

@ibraheemdev
Copy link
Member

Seems they are on vacation. r? libs-api because from what I understand this adds additional guarantees to the public API.

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 31, 2025
@rustbot rustbot assigned dtolnay and unassigned ibraheemdev Jan 31, 2025
@dtolnay dtolnay removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jan 31, 2025
@dtolnay
Copy link
Member

dtolnay commented Jan 31, 2025

@rust-lang/libs-api:
@rfcbot fcp merge

This PR adds the guarantee that vec! and Vec::with_capacity request an allocation of the exact size needed for that number of elements (as long as size_of::<T>() * n > 0 as carved out in a previous paragraph).

Whether the actual allocation can hold a larger number of elements is up to the allocator. So this still only guarantees that vec.capacity() >= n, not ==.

Before this PR, we guaranteed vec.capacity() >= n but did not guarantee that n would be the requested size of the allocation. Hypothetically the Vec implementation could have rounded up the capacity, and that will no longer be allowed. Only the allocator gets to round up.

@rfcbot
Copy link

rfcbot commented Jan 31, 2025

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 31, 2025
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 11, 2025
@rfcbot
Copy link

rfcbot commented Feb 11, 2025

🔔 This is now entering its final comment period, as per the review above. 🔔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removal of exact capacity guarantee for Vec::with_capacity() is a breaking change
7 participants