Skip to content

Clean up clean/types.rs file by making the implementations follow the type declaration #89203

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

Merged
merged 1 commit into from
Sep 27, 2021

Conversation

GuillaumeGomez
Copy link
Member

This PR doesn't change anything, it simply moves things around: when reading the code, I realized a few times that a type declaration and implementations on it might be separated by some other type declarations, which makes the reading much more complicated. I put back impl and declaration together.

r? @camelid

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 23, 2021
@GuillaumeGomez GuillaumeGomez added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 23, 2021
@camelid camelid added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 23, 2021
Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

There's a cost-benefit ratio to moving code: the cost of the Git churn (primarily merge conflicts, but also making Git blame more annoying to use), and the benefit of the new version of the code.

Some of the changes here feel to me like the benefit outweighs the cost (e.g., for small sections of code, like Attributes, that also aren't changed that often), while others feel like the cost outweighs the benefit (e.g., moving Type and its impls, which are large amounts of code that change somewhat frequently, and which I'm currently in the process of refactoring).

I left comments on the main parts where I feel the cost outweighs the benefit and asked to revert them.

Comment on lines 1304 to 1316
#[derive(Clone, PartialEq, Eq, Debug, Hash)]
crate struct Argument {
crate type_: Type,
crate name: Symbol,
}

#[derive(Clone, PartialEq, Debug)]
crate enum SelfTy {
SelfValue,
SelfBorrowed(Option<Lifetime>, Mutability),
SelfExplicit(Type),
}

#[derive(Clone, PartialEq, Eq, Debug, Hash)]
crate struct Argument {
crate type_: Type,
crate name: Symbol,
}

Copy link
Member

Choose a reason for hiding this comment

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

Can you leave this part as it was before? Both Argument and SelfTy are logically connected to the following impl, so this feels like unnecessary churn. I'm likely going to be removing SelfTy soon anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh nice!

Comment on lines 1378 to 1382
/// A representation of a type suitable for hyperlinking purposes. Ideally, one can get the original
/// type out of the AST/`TyCtxt` given one of these, if more information is needed. Most
/// importantly, it does not preserve mutability or boxes.
#[derive(Clone, PartialEq, Eq, Debug, Hash)]
crate enum Type {
Copy link
Member

Choose a reason for hiding this comment

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

Moving Type and its impls will cause a lot of conflicts with my "Cleanup clean" PR and probably other PRs as well. Can you please revert this part of the change? I'm okay with keeping the merging of the two inherent impls on Type (the general one and the inner_def_id one), and maybe also moving trait GetDefId { ... } so it's not in the middle of the Type code, but most of the other changes feel like unnecessary churn to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok!

@camelid camelid 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 Sep 23, 2021
@GuillaumeGomez GuillaumeGomez added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 23, 2021
@GuillaumeGomez
Copy link
Member Author

Done!

@GuillaumeGomez
Copy link
Member Author

Done as well.

@camelid
Copy link
Member

camelid commented Sep 23, 2021

Thanks!

@bors r+ rollup=iffy

(iffy because it has some potential for merge conflicts)

@bors
Copy link
Collaborator

bors commented Sep 23, 2021

📌 Commit 1c23349 has been approved by camelid

@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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 23, 2021
@jyn514
Copy link
Member

jyn514 commented Sep 23, 2021

@camelid bors will automatically keep the PR out of the rollup if it conflicts with other PRs, so there's no need to mark it as iffy.

@bors rollup=maybe

@bors
Copy link
Collaborator

bors commented Sep 27, 2021

⌛ Testing commit 1c23349 with merge 583437a...

@bors
Copy link
Collaborator

bors commented Sep 27, 2021

☀️ Test successful - checks-actions
Approved by: camelid
Pushing 583437a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 27, 2021
@bors bors merged commit 583437a into rust-lang:master Sep 27, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 27, 2021
@GuillaumeGomez GuillaumeGomez deleted the cleanup-rustdoc-types branch September 27, 2021 11:06
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (583437a): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. merged-by-bors This PR was explicitly merged by bors. 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
Development

Successfully merging this pull request may close these issues.

7 participants