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

Remove under-used ImplPolarity enum #80825

Merged
merged 2 commits into from
Jan 10, 2021
Merged

Remove under-used ImplPolarity enum #80825

merged 2 commits into from
Jan 10, 2021

Conversation

GuillaumeGomez
Copy link
Member

It doesn't make much sense to have an enum with only two possible values and to store it inside an Option in my opinion when you can do all the same with a simple boolean. I don't expect any chances, performance or RSS usage wise.

r? @jyn514

@GuillaumeGomez GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 8, 2021
@GuillaumeGomez GuillaumeGomez force-pushed the rustdoc-cleanup-bis-repetita branch from 499e65f to 28e8daf Compare January 8, 2021 21:59
@cjgillot
Copy link
Contributor

cjgillot commented Jan 8, 2021

Could the similar rustc_ast::ast::ImplPolarity enum be reused? This would bring rustdoc IR closer to the backing HIR.

@GuillaumeGomez
Copy link
Member Author

The only one we seem to interact with is rustc_middle::ty::ImplPolarity. So not sure if it'd be a good idea to bring another type from earlier stages.

@jyn514
Copy link
Member

jyn514 commented Jan 8, 2021

What does an impl polarity of None currently mean?

I don't think it's clear at all that true and false mean 'positive impl' and 'negative impl'. I would rather have an enum for that, at which point I think it makes more sense to use ast::Polarity or ty::Polarity.

@GuillaumeGomez
Copy link
Member Author

What does an impl polarity of None currently mean?

It's treated as "non-negative impl". Which is why I turned it into a boolean.

I don't think it's clear at all that true and false mean 'positive impl' and 'negative impl'. I would rather have an enum for that, at which point I think it makes more sense to use ast::Polarity or ty::Polarity.

If you prefer. I think it's a bit too much considering how little use of it we make. Don't you confirm your choice? ;)

@GuillaumeGomez GuillaumeGomez force-pushed the rustdoc-cleanup-bis-repetita branch from 28e8daf to d6c35b0 Compare January 8, 2021 23:21
@GuillaumeGomez
Copy link
Member Author

After a talk, we agreed on keeping the boolean but to rename the field from polarity to negative_polarity to make it more obvious what this was about.

jyn514
jyn514 reviewed Jan 8, 2021
View reviewed changes
src/librustdoc/clean/blanket_impl.rs Show resolved Hide resolved
src/librustdoc/clean/mod.rs Show resolved Hide resolved
src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 8, 2021
@GuillaumeGomez GuillaumeGomez force-pushed the rustdoc-cleanup-bis-repetita branch from d6c35b0 to 34d128a Compare January 9, 2021 13:59
@jyn514
Copy link
Member

jyn514 commented Jan 9, 2021

r=me with CI passing

@GuillaumeGomez
Copy link
Member Author

@bors: r=jyn514

@bors
Copy link
Contributor

bors commented Jan 9, 2021

📌 Commit 34d128a has been approved by jyn514

@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 Jan 9, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 10, 2021
@JohnTitor
Rollup merge of rust-lang#80825 - GuillaumeGomez:rustdoc-cleanup-bis-…
8386a57 
…repetita, r=jyn514

Remove under-used ImplPolarity enum

It doesn't make much sense to have an enum with only two possible values and to store it inside an `Option` in my opinion when you can do all the same with a simple boolean. I don't expect any chances, performance or RSS usage wise.

r? `@jyn514`
This was referenced Jan 10, 2021
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 10, 2021
@bors
Auto merge of rust-lang#80867 - JohnTitor:rollup-tvqw555, r=JohnTitor
34628e5 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#79502 (Implement From<char> for u64 and u128.)
 - rust-lang#79968 (Improve core::ptr::drop_in_place debuginfo)
 - rust-lang#80774 (Fix safety comment)
 - rust-lang#80801 (Use correct span for structured suggestion)
 - rust-lang#80803 (Remove useless `fill_in` function)
 - rust-lang#80820 (Support `download-ci-llvm` on NixOS)
 - rust-lang#80825 (Remove under-used ImplPolarity enum)
 - rust-lang#80850 (Allow #[rustc_builtin_macro = "name"])
 - rust-lang#80857 (Add comment to `Vec::truncate` explaining `>` vs `>=`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5acac4c into rust-lang:master Jan 10, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 10, 2021
@GuillaumeGomez GuillaumeGomez deleted the rustdoc-cleanup-bis-repetita branch January 10, 2021 12:10
@jyn514 jyn514 mentioned this pull request Mar 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

Assignees
No one assigned
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.51.0
Development

Successfully merging this pull request may close these issues.
5 participants
@GuillaumeGomez @cjgillot @jyn514 @bors @rustbot