Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make match in register_res easier to read #84880

Merged
merged 1 commit into from
Jun 5, 2021
Merged

Make match in register_res easier to read #84880

merged 1 commit into from
Jun 5, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented May 3, 2021

  • Don't duplicate DefKind -> ItemType handling; that's a good way to get bugs
  • Use exhaustive match
  • Add comments

This found that register_res is very wrong in at least one way: if it
registers a Res for Variant, it should also register one for Field.
But I don't know whether the one for Variant should be removed or Field
added. Maybe someone has ideas?

Found while reviewing #84176.

@rust-highfive
Copy link
Collaborator

r? @ollie27

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 3, 2021
@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels May 3, 2021
@jyn514 jyn514 force-pushed the cleanup-itemkind branch from 3764a53 to 2e0d9aa Compare May 3, 2021 19:40
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jyn514 jyn514 force-pushed the cleanup-itemkind branch from 2e0d9aa to b98316f Compare May 3, 2021 19:54
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jyn514
Make match in register_res easier to read
4029a03 
- Don't duplicate DefKind -> ItemType handling; that's a good way to get bugs
- Use exhaustive match
- Add comments

This found that register_res is very wrong in at least one way: if it
registers a Res for `Variant`, it should also register one for `Field`.
But I don't know whether the one for Variant should be removed or Field
added. Maybe someone has ideas?
@jyn514 jyn514 force-pushed the cleanup-itemkind branch from b98316f to 4029a03 Compare May 3, 2021 21:01
@jyn514 jyn514 mentioned this pull request May 3, 2021
Merged
GuillaumeGomez
GuillaumeGomez reviewed May 4, 2021
View reviewed changes
src/test/rustdoc/intra-doc/field.rs
@@ -0,0 +1,4 @@
// @has field/index.html '//a[@href="https://doc.rust-lang.org/nightly/core/ops/range/struct.Range.html#structfield.start"]' 'start'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicking: generally we tend to put such checks below the inner attributes. Not a blocker though.

GuillaumeGomez
GuillaumeGomez approved these changes May 4, 2021
View reviewed changes
Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the nitpick, looks good to me. r=me with or without it.

@jyn514
Copy link
Member Author

jyn514 commented May 4, 2021

Thanks for the review :) do you have ideas about this bit?

This found that register_res is very wrong in at least one way: if it
registers a Res for Variant, it should also register one for Field.
But I don't know whether the one for Variant should be removed or Field
added. Maybe someone has ideas?

@GuillaumeGomez
Copy link
Member

Fields and variants are different: a variant can represent its enum whereas a field cannot represent its struct. So it seems logical that there is res for one but not for the other.

@jyn514
Copy link
Member Author

jyn514 commented May 4, 2021

a variant can represent its enum whereas a field cannot represent its struct

What does this mean?

@GuillaumeGomez
Copy link
Member

I'm sorry, I'm really bad at explaining things... What I tried to say is that it's logical for variant to have Res because they are a whole type themselves. Unlike fields which don't represent the struct.

Well, I guess that explanation doesn't make any sense either... XD

@jyn514
Copy link
Member Author

jyn514 commented May 4, 2021

@GuillaumeGomez maybe this is easier to answer - can you write a test case that fails if we ignore Variant? Or that fails currently because we ignore Field?

@GuillaumeGomez
Copy link
Member

After re-reading, I think I misunderstood and that we should have Res for both variants and fields.

@bstrie
Copy link
Contributor

bstrie commented May 19, 2021

I'm unclear about the status of this, is it reviewed, approved, waiting for changes?

@jyn514
Copy link
Member Author

jyn514 commented May 19, 2021

@bstrie it's probably ok to approve as is, but I'd like to figure out what's going on with Field first, and ideally write a test case for it. It's unclear when this code path ever makes a difference.

@bstrie bstrie added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2021
@jyn514
Copy link
Member Author

jyn514 commented Jun 4, 2021

I won't have time to investigate this in the near future. I'll open an issue for the fishiness with Field.

@bors r=GuillaumeGomez rollup

@bors
Copy link
Contributor

bors commented Jun 4, 2021

📌 Commit 4029a03 has been approved by GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 4, 2021
@JohnTitor JohnTitor mentioned this pull request Jun 4, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2021
@bors
Auto merge of rust-lang#86006 - JohnTitor:rollup-97iuoi3, r=JohnTitor
704934d 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#83653 (Remove unused code from `rustc_data_structures::sync`)
 - rust-lang#84466 (rustdoc: Remove `PrimitiveType::{to_url_str, as_str}`)
 - rust-lang#84880 (Make match in `register_res` easier to read)
 - rust-lang#84942 (rustdoc: link to stable/beta docs consistently in documentation)
 - rust-lang#85853 (Warn against boxed DST in `improper_ctypes_definitions` lint)
 - rust-lang#85939 (Fix suggestion for removing &mut from &mut macro!().)
 - rust-lang#85966 (wasm: Make simd types passed via indirection again)
 - rust-lang#85979 (don't suggest unsized indirection in where-clauses)
 - rust-lang#85983 (Update to semver 1.0.3)
 - rust-lang#85988 (Note that `ninja = false` goes under `[llvm]`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3a8bb38 into rust-lang:master Jun 5, 2021
@rustbot rustbot added this to the 1.54.0 milestone Jun 5, 2021
@jyn514 jyn514 deleted the cleanup-itemkind branch June 5, 2021 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@ollie27 ollie27

Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.54.0
Development

Successfully merging this pull request may close these issues.
8 participants
@jyn514 @rust-highfive @rust-log-analyzer @GuillaumeGomez @bstrie @bors @ollie27 @rustbot