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

Improve panic messages for Vec methods #70524

Closed
IgorPerikov opened this issue Mar 29, 2020 · 5 comments · Fixed by #70573
Closed

Improve panic messages for Vec methods #70524

IgorPerikov opened this issue Mar 29, 2020 · 5 comments · Fixed by #70573
Labels
A-collections Area: std::collections. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@IgorPerikov
Copy link
Contributor

I've spotted, that remove() panic message could've provided details on what values led to this panic, instead of mentioning condition only. Generally speaking this and some other assert!s in Vec could've been more detailed. If it's okay to step in, I'd like to offer my help.

In case of remove I'd suggest calling this instead

assert!(index < len, "removal index {} should be less than length {}", index, len); 

I find this more informative than assertion failed: index < len

Candidates for improval:

aforementioned remove:

assert!(index < len);

insert:

assert!(index <= len);

drain:

rust/src/liballoc/vec.rs

Lines 1292 to 1293 in 5f13820

assert!(start <= end);
assert!(end <= len);

split_off:

assert!(at <= self.len(), "`at` out of bounds");

@Centril Centril added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-collections Area: std::collections. labels Mar 29, 2020
@IgorPerikov
Copy link
Contributor Author

@Centril Does triaging mean a green light for this suggestion?

@Centril
Copy link
Contributor

Centril commented Mar 29, 2020

@IgorPerikov No, I just apply the relevant labels. :)

@Centril
Copy link
Contributor

Centril commented Mar 30, 2020

@IgorPerikov Though this is a small change with little time required to fix, so I'd just make the PR if I were you. :)

@RalfJung
Copy link
Member

Note: #70558 uses the same kind of assertion in swap_remove (as the old check had aliasing issues).

Depending on which PR lands first, we should make sure it gets the same panic message as the others.

@IgorPerikov
Copy link
Contributor Author

@RalfJung I think it makes sense to follow the message, that is being currently printed, when trying to access element out of bounds via []. Current message template is index out of bounds: the len is {len} but the index is {index}. I followed this idiom to keep it consistent.

@bors bors closed this as completed in 6dee5f1 Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: std::collections. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants