Use named fields for {ast,hir}::ItemKind::Impl
#68204
Merged
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
|
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 |
ecstatic-morse
force-pushed
the
item-kind-impl
branch
from
27fdce1
to
134692b
Compare
JohnTitor
added
the
S-waiting-on-review
|
r? @oli-obk |
|
Seems fine to me; just make sure to use exhaustive matching where appropriate by enumerating the fields with |
Centril
reviewed
Jan 15, 2020
| polarity: _, | ||
| defaultness: _, | ||
| ref generics, | ||
| ref of_trait, |
There was a problem hiding this comment.
Not sure what "of" signifies here. I would rename this to trait_ref and let the type system speak to the Option<_>
There was a problem hiding this comment.
It's Some if the item is an impl of a trait.
There was a problem hiding this comment.
But what does the f stand for? (I know what the field means.)
There was a problem hiding this comment.
"of" like the preposition. An impl of a trait.
There was a problem hiding this comment.
Oh. Well that was far from obvious. :) Given that you had to ELI5 it for me, could we change it to something else?
There was a problem hiding this comment.
I could, although to be honest I'm not enthusiastic about it. On a personal note, I have less time to contribute to open source this year, and having to spend some of it arguing over trivial choices like this has left me feeling a bit bummed out 😢.
edit: I'm aware that "arguing" requires two parties, so I'm partly responsible for using my time this way. I suppose I just don't see the value in this kind of extremely granular review.
There was a problem hiding this comment.
(Fwiw, I didn't think we were arguing -- I was just confused as to what it signified.)
|
@bors r+ |
|
📌 Commit 134692b904dcbb12ca8d15d96bba4c7ca487e4e3 has been approved by |
bors
added
S-waiting-on-bors
|
@bors r- oh overlooked the |
bors
added
S-waiting-on-author
|
@bors r+ while i did stumble over |
|
📌 Commit 134692b904dcbb12ca8d15d96bba4c7ca487e4e3 has been approved by |
bors
added
S-waiting-on-bors
|
In case this bitrots, rebase and r=me with p=1 |
bors
added a commit
that referenced
this pull request
Jan 16, 2020
Rollup of 5 pull requests Successful merges: - #67780 (Move some queries from rustc::ty to librustc_ty.) - #68096 (Clean up some diagnostics by making them more consistent) - #68223 (Use 3.6 instead of 3.5 in float fract() documentation) - #68265 (Fix some issue numbers of unstable features) - #68266 (Changed docs for f32 and f64.) Failed merges: - #68204 (Use named fields for `{ast,hir}::ItemKind::Impl`) r? @ghost
|
☔ The latest upstream changes (presumably #68272) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
|
@bors p=1 |
ecstatic-morse
force-pushed
the
item-kind-impl
branch
from
134692b
to
758860c
Compare
|
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
ecstatic-morse
force-pushed
the
item-kind-impl
branch
from
758860c
to
4743995
Compare
|
@bors r=oli-obk |
|
📌 Commit 4743995 has been approved by |
bors
added
S-waiting-on-bors
tmandry
added a commit
to tmandry/rust
that referenced
this pull request
Jan 18, 2020
…i-obk
Use named fields for `{ast,hir}::ItemKind::Impl`
Currently, the widely used `ItemKind::Impl` variant is a tuple with seven fields. I want to add an eighth in rust-lang#68140, which means I have to update most matches on this variant anyways. Giving a name to each field improves readability and makes future changes of this nature much simpler.
This change will cause several tools to break. I will fix them once this is merged.
bors
added a commit
that referenced
this pull request
Jan 18, 2020
Rollup of 9 pull requests Successful merges: - #66660 (Don't warn about snake case for field puns.) - #68093 (Fix deref impl typedef) - #68204 (Use named fields for `{ast,hir}::ItemKind::Impl`) - #68256 (Do not ICE on malformed suggestion spans) - #68279 (Clean up E0198 explanation) - #68291 (Update sanitizer tests) - #68312 (Add regression test for integer literals in generic arguments in where clauses) - #68314 (Stop treating `FalseEdges` and `FalseUnwind` as having semantic value for const eval) - #68317 (Clean up E0199 explanation) Failed merges: r? @ghost
JohnTitor
added a commit
to JohnTitor/rust-clippy
that referenced
this pull request
Jan 18, 2020
JohnTitor
added a commit
to JohnTitor/rust-clippy
that referenced
this pull request
Jan 18, 2020
ecstatic-morse
added a commit
to ecstatic-morse/rust-clippy
that referenced
this pull request
Jan 18, 2020
`{ast,hir}::ItemKind::Impl` use named fields now
bors
added a commit
to rust-lang/rust-clippy
that referenced
this pull request
Jan 18, 2020
Rustup to rust-lang/rust#68204 changelog: none
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 18, 2020
Changes: ```` Downgrade range_plus_one to pedantic Rustup to rust-lang#68204 Add lifetimes to `LateLintPass` Fix rustc lint import paths generated by `new_lint` Add lint for default lint description Update documentation for adding new lints Generate new lints easily ````
bors
added a commit
that referenced
this pull request
Jan 18, 2020
submodules: update clippy from a8d90f6 to 7ae2442 Changes: ```` Downgrade range_plus_one to pedantic Rustup to #68204 Add lifetimes to `LateLintPass` Fix rustc lint import paths generated by `new_lint` Add lint for default lint description Update documentation for adding new lints Generate new lints easily ```` Fixes #68331
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 19, 2020
Changes: ```` Treat more strange pattern Split up `if_same_then_else` ui test Apply review comments Run `update_lints` Reduce span range Rename `ok_if_let` to `if_let_some_result` Apply review comments Add suggestion in `if_let_some_result` rustup rust-lang#67712 Allow `unused_self` lint at the function level Downgrade range_plus_one to pedantic Rustup to rust-lang#68204 Add lifetimes to `LateLintPass` Fix rustc lint import paths generated by `new_lint` Add lint for default lint description Update documentation for adding new lints Generate new lints easily Split up `booleans` ui test Fix the ordering on `nonminimal_bool` ````
bors
added a commit
that referenced
this pull request
Jan 19, 2020
submodules: update clippy from a8d90f6 to 7ae2442 Changes: ```` Downgrade range_plus_one to pedantic Rustup to #68204 Add lifetimes to `LateLintPass` Fix rustc lint import paths generated by `new_lint` Add lint for default lint description Update documentation for adding new lints Generate new lints easily ```` Fixes #68331
flip1995
pushed a commit
to flip1995/rust-clippy
that referenced
this pull request
May 5, 2020
Changes: ```` Treat more strange pattern Split up `if_same_then_else` ui test Apply review comments Run `update_lints` Reduce span range Rename `ok_if_let` to `if_let_some_result` Apply review comments Add suggestion in `if_let_some_result` rustup rust-lang/rust#67712 Allow `unused_self` lint at the function level Downgrade range_plus_one to pedantic Rustup to rust-lang/rust#68204 Add lifetimes to `LateLintPass` Fix rustc lint import paths generated by `new_lint` Add lint for default lint description Update documentation for adding new lints Generate new lints easily Split up `booleans` ui test Fix the ordering on `nonminimal_bool` ````
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.
Currently, the widely used
ItemKind::Implvariant is a tuple with seven fields. I want to add an eighth in #68140, which means I have to update most matches on this variant anyways. Giving a name to each field improves readability and makes future changes of this nature much simpler.This change will cause several tools to break. I will fix them once this is merged.