Type dependent defs wrappers #59216
Merged
Type dependent defs wrappers #59216
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @oli-obk |
oli-obk
requested changes
Mar 15, 2019
There was a problem hiding this comment.
This won't break clippy, so there's no immediate worry. If you want to fix clippy, too, you can just open a normal PR against it once this PR has been merged.
Just a few nits then this PR is ready to go.
Please remember to rebase and not merge. Your merge commits should automatically disappear if you rebase.
src/librustc/ty/context.rs
Outdated
| @@ -482,6 +482,23 @@ impl<'tcx> TypeckTables<'tcx> { | |||
| } | |||
| } | |||
|
|
|||
| pub fn opt_type_dependent_def(&self, id: HirId) -> Option<&Def> { | |||
There was a problem hiding this comment.
you could return Def instead of &Def (by using .copied() inside this function)
There was a problem hiding this comment.
This should allow you to save all the .cloned() calls after the opt_type_dependent_def calls.
src/librustc/ty/context.rs
Outdated
| } | ||
|
|
||
| pub fn opt_type_dependent_def_id(&self, id: HirId) -> Option<DefId> { | ||
| match self.opt_type_dependent_def(id) { |
There was a problem hiding this comment.
Remove this match by using the map method on the result of opt_type_dependent_def
|
I'm a bit skeptical about a cleanup that adds more code than removes. |
|
@petrochenkov everything (besides mentioned clippy) should be converted. For me this update makes sense as it provides a bit clener iface and callers of these methods don't have to worry about internal implementations/details. |
oli-obk
reviewed
Mar 15, 2019
src/librustc/ty/context.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn type_dependent_def_id(&self, id: HirId) -> DefId { |
There was a problem hiding this comment.
since this seems unused, you can remove it and have clippy just unwrap the opt_type_dependent_def_id (and maybe rename that method to drop the opt_)
I assumed that reducing the capabilities (since we'll be able to make |
|
r? @oli-obk |
|
The code lgtm now. Can you rebase and sqash your commits? |
oli-obk
approved these changes
Mar 16, 2019
|
@oli-obk done, I had to force push. sry haven't seen rebase convetion mentioned anywhere. |
I just checked, and we do indeed not mention it anywhere. We should. I'll open an issue about that |
|
@bors r+ |
|
📌 Commit f4742f4 has been approved by |
bors
added
the
S-waiting-on-bors
Mark-Simulacrum
added a commit
to Mark-Simulacrum/rust
that referenced
this pull request
Mar 23, 2019
…ers, r=oli-obk Type dependent defs wrappers First of all, forgive me if something would seem lame to you or I offend some rule (although I tried to read through docs), this is my first PR. Issue: rust-lang#59094 This PR adds 3 helper methods to `TypeckTables`: * `opt_type_dependent_def` * `opt_type_dependent_def_id` * `type_dependent_def_id` I didn't add `type_dependent_def` as was proposed in the issue simply because it wasn't used anywhere in the code. Only non-option wrapped`type_dependent_defs()[]` accesses were found in clippy which always called `def_id()` on result. Speaking of clippy, should I open separate PR in its own repo, given it's used as submodule here? Sry it took me so long, as I said I'm new here and I had tough week :).
|
⌛ Testing commit f4742f4 with merge 8a1b866eaa1b38a62904ac417ce0dfe72fa858cc... |
|
💔 Test failed - checks-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Mar 28, 2019
…ers, r=oli-obk Type dependent defs wrappers First of all, forgive me if something would seem lame to you or I offend some rule (although I tried to read through docs), this is my first PR. Issue: rust-lang#59094 This PR adds 3 helper methods to `TypeckTables`: * `opt_type_dependent_def` * `opt_type_dependent_def_id` * `type_dependent_def_id` I didn't add `type_dependent_def` as was proposed in the issue simply because it wasn't used anywhere in the code. Only non-option wrapped`type_dependent_defs()[]` accesses were found in clippy which always called `def_id()` on result. Speaking of clippy, should I open separate PR in its own repo, given it's used as submodule here? Sry it took me so long, as I said I'm new here and I had tough week :).
bors
added a commit
that referenced
this pull request
Mar 28, 2019
Rollup of 10 pull requests Successful merges: - #58717 (Add FromStr impl for NonZero types) - #59091 (Combine input and eval_always query types) - #59216 (Type dependent defs wrappers) - #59318 (rustc: Update linker flavor inference from filename) - #59320 (rustc: Allow using `clang` for wasm32 targets) - #59363 (#59361 Moved rustc edition opt to short list) - #59371 (ffi: rename VaList::copy to VaList::with_copy) - #59398 (Add a way to track Rustfix UI test coverage) - #59408 (compiletest: make path normalization smarter) - #59429 (When moving out of a for loop head, suggest borrowing it in nll mode) Failed merges: r? @ghost
bors
added a commit
to rust-lang/rust-clippy
that referenced
this pull request
Apr 10, 2019
Support updated type dependent def ID getter This PR changes all the calls to `type_dependent_defs` (with panics via `[]`) to new method `type_dependent_def_id` with more explicit `unwrap` panic. Rust PR: rust-lang/rust#59216 Issue author: @oli-obk
bors
added a commit
to rust-lang/rust-clippy
that referenced
this pull request
Apr 10, 2019
Support updated type dependent def ID getter This PR changes all the calls to `type_dependent_defs` (with panics via `[]`) to new method `type_dependent_def_id` with more explicit `unwrap` panic. Rust PR: rust-lang/rust#59216 Issue author: @oli-obk
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First of all, forgive me if something would seem lame to you or I offend some rule (although I tried to read through docs), this is my first PR.
Issue: #59094
This PR adds 3 helper methods to
TypeckTables:opt_type_dependent_defopt_type_dependent_def_idtype_dependent_def_idI didn't add
type_dependent_defas was proposed in the issue simply because it wasn't used anywhere in the code. Only non-option wrappedtype_dependent_defs()[]accesses were found in clippy which always calleddef_id()on result.Speaking of clippy, should I open separate PR in its own repo, given it's used as submodule here?
Sry it took me so long, as I said I'm new here and I had tough week :).