-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Use get_diagnostic_name
more
#90985
Use get_diagnostic_name
more
#90985
Conversation
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
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 a cursory glance. Need a second look before approving.
}, | ||
BorrowedContentSource::OverloadedDeref(ty) => ty | ||
.ty_adt_def() | ||
.and_then(|adt| match tcx.get_diagnostic_name(adt.did)? { |
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.
Kind of a nit, but I think I prefer having a separate and_then
for get_diagnostic_name
, rather than using the question mark.
}, | ||
BorrowedContentSource::OverloadedDeref(ty) => ty | ||
.ty_adt_def() | ||
.and_then(|adt| match tcx.get_diagnostic_name(adt.did)? { |
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.
Same here.
for &i in | ||
&[sym::std_panic_macro, sym::core_panic_macro, sym::assert_macro, sym::debug_assert_macro] | ||
{ | ||
loop { |
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...isn't exactly the same, right?
Previously, we can only match each diagnostic item once? But now we can match the same on continously?
Also, now we can ascend many times.
I'm not sure there are actually any semantic changes, just worth mentioning.
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.
In the current implementation, I think the order of the list is significant because assert
contains a call to panic
, and we want to climb to the outermost macro call. This implementation works differently but should have the same result AFAICT.
r=me with or without nit @bors delegate+ |
✌️ @camsteffen can now approve this pull request |
@bors r=jackh726 |
📌 Commit dec4053 has been approved by |
Use `get_diagnostic_name` more
Use `get_diagnostic_name` more
Use `get_diagnostic_name` more
…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#87160 (When recovering from a `:` in a pattern, use adequate AST pattern) - rust-lang#90985 (Use `get_diagnostic_name` more) - rust-lang#91087 (Remove all migrate.nll.stderr files) - rust-lang#91207 (Add support for LLVM coverage mapping format versions 5 and 6) - rust-lang#91298 (Improve error message for `E0659` if the source is not available) - rust-lang#91346 (Add `Option::inspect` and `Result::{inspect, inspect_err}`) - rust-lang#91404 (Fix bad `NodeId` limit checking.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Use let_else in some more places in rustc_lint Follow-up of rust-lang#91018 and rust-lang#89933 . Also cc rust-lang#90985 which added the first let_else uses to rustc_lint.
No description provided.