-
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
Remove use of unwrap()
from save-analysis
#69422
Conversation
r? @davidtwco (rust_highfive has picked a reviewer for you, use r? to override) |
r? @Xanewok |
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 brings it in line with other usages and is safe to do, left some further comments
src/librustc_save_analysis/lib.rs
Outdated
span, | ||
ref_id: id_from_def_id(variant.fields[index].did), | ||
})); | ||
if let Some(index) = self.tcx.find_field_index(ident, variant) { |
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.
Could we use map
here? Since we indent by a level anyway and to avoid trailing None
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.
Addressed via a211a82.
src/test/ui/assign-to-method.rs
Outdated
@@ -1,3 +1,5 @@ | |||
// compile-flags: -Zsave-analysis |
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.
I think we should have a dedicated regression test in src/test/ui/save-analysis
and not rely on this flag in general src/test/ui
directory
cc @Centril on test layout
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.
My general sentiment is that we should not have files at all in src/test/ui
, only directories. I have a question though... is the purpose of this test just for save-analysis? If not, then it doesn't seem fit for a save-analysis/
folder, which I would expect to be targeted specifically at testing save-analysis as opposed to some other aspect (and save analysis being incidentally also tested).
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.
Left comments to clarify and moved assign-to-method.rs
to ui/methods
.
@@ -1,3 +1,5 @@ | |||
// compile-flags: -Zsave-analysis |
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.
Ditto
2aeaba3
to
5307edc
Compare
Friendly ping @Xanewok, addressed your reviews, how about? |
Sorry, forgot about this! Thanks =) @bors r+ rollup |
📌 Commit 5307edc has been approved by |
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened |
Remove use of `unwrap()` from save-analysis Fix rust-lang#69409, fix rust-lang#69416
Rollup of 8 pull requests Successful merges: - #69422 (Remove use of `unwrap()` from save-analysis) - #69548 (Turn trailing tokens in `assert!()` into hard errors) - #69561 (Clean up unstable book) - #69599 (check_binding_alt_eq_ty: improve precision wrt. `if let`) - #69641 (Update books) - #69776 (Fix & test leak of some BTreeMap nodes on panic during `into_iter`) - #69805 (resolve: Modernize some naming) - #69810 (test(bindings_after_at): add dynamic drop tests for bindings_after_at) Failed merges: r? @ghost
Fix #69409, fix #69416