-
Notifications
You must be signed in to change notification settings - Fork 1.8k
refactor(missing_asserts_for_indexing): overhaul diagnostics #16120
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
base: master
Are you sure you want to change the base?
Conversation
|
Lintcheck changes for 129a39d
This comment will be updated if you push new changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the labels need to be removed. Looks good otherwise.
| diag.span_note(*span, "slice indexed here"); | ||
| } | ||
| diag.note("asserting the length before indexing will elide bounds checks"); | ||
| diag.span_labels(index_spans, "slice indexed here"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This label adds more noise than anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the lint spans are sufficient on their own now
| | ^^^^ ^^^^ ^^^^ ^^^^ ^^^^ slice indexed here | ||
| | | | | | | ||
| | | | | slice indexed here | ||
| | | | slice indexed here | ||
| | | slice indexed here | ||
| | slice indexed here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the previous. This is several lines cluttering the diagnostic while not adding new information.
|
Reminder, once the PR becomes ready for a review, use |
This is a general advice, and so shouldn't be repeated
This makes the labels redundant.
bc76aef to
129a39d
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@rustbot ready |
changelog: [
missing_asserts_for_indexing]: overhaul diagnostics