Skip to content

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Mar 22, 2022

Neither the number of elements in a vector can overflow a usize, nor
can the amount of elements in two vectors.

@rust-highfive
Copy link
Contributor

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 22, 2022
Neither the number of elements in a vector can overflow a `usize`, nor
can the amount of elements in two vectors.
@tbu- tbu- force-pushed the pr_vec_append_doc branch from cd497a6 to 4123d33 Compare March 22, 2022 20:08
@marmeladema
Copy link
Contributor

What about ZST?

@tbu-
Copy link
Contributor Author

tbu- commented Mar 23, 2022

All comments about ZST apply to Vec::push equally as well. Should I add a note here and to Vec::push about zero-sized types?

@Mark-Simulacrum
Copy link
Member

I'm not sure I understand how ZST types are relevant here; I'm not seeing code that will panic on ZST types related to isize::MAX.

This change seems OK to me though, since it matches the reserve(...) panic note, and AFAICT that's the only source of panics for this method as well.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 6, 2022
@tbu-
Copy link
Contributor Author

tbu- commented Apr 6, 2022

I don't see anything to do for me. Did you add "S-waiting-on-author" by accident? @Mark-Simulacrum

@@ -1767,7 +1767,7 @@ impl<T, A: Allocator> Vec<T, A> {
///
/// # Panics
///
/// Panics if the number of elements in the vector overflows a `usize`.
/// Panics if the new capacity exceeds `isize::MAX` bytes.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the wording here (and in Vec::push) is confusing because it uses the term capacity which in this case is not related to the Vec::capacity method since for a ZST, Vec::capacity can be bigger than isize::MAX but no actual memory will be allocated:

fn main() {
        let mut vec = vec![(); usize::MAX];
        assert_eq!(vec.len(), usize::MAX);
        assert_eq!(vec.capacity(), usize::MAX);
        assert!(vec.capacity() > usize::try_from(isize::MAX).unwrap());
        vec.append(&mut vec![]);
        assert!(vec.capacity() > usize::try_from(isize::MAX).unwrap());
}

https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=b75b2d890ed0f530ca925ff424f4246c

I would rather change both panic notes to use a different terminology like allocated memory with something for ZST that says no memory will be allocated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: It's also not directly related to the Vec::capacity method for other types, it's just for 1-byte large types where it corresponds 1-to-1. E.g. a 2-byte type can at most have isize::MAX / 2 capacity.

What replacement would you suggest?

Panics if the memory to be allocated would exceed isize::MAX bytes.

It seems to me that this would introduce another terminology and would thus make the note harder to understand.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 23, 2022
@tbu-
Copy link
Contributor Author

tbu- commented Apr 27, 2022

@JohnCSimon On what action of the author is the PR waiting on?

@JohnCSimon
Copy link
Member

@tbu- triage just bumps the PR by adding + removing the tag so it shows that it was updated.

You can message the reviewer for clarification or talk on zulip.

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

@tbu-
Copy link
Contributor Author

tbu- commented May 23, 2022

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 23, 2022
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 23, 2022
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

There may be further improvements to the language here but I don't think anything is blocking this PR at this point, so I'm going to go ahead and approve.

@bors
Copy link
Collaborator

bors commented May 27, 2022

📌 Commit 4123d33 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 27, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request May 27, 2022
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#95214 (Remove impossible panic note from `Vec::append`)
 - rust-lang#97411 (Print stderr consistently)
 - rust-lang#97453 (rename `TyKind` to `RegionKind` in comment in rustc_middle)
 - rust-lang#97457 (Add regression test for rust-lang#81899)
 - rust-lang#97458 (Modify `derive(Debug)` to use `Self` in struct literal to avoid redundant error)
 - rust-lang#97462 (Add more eslint rules)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4254f92 into rust-lang:master May 28, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants