-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
Conversation
There was a problem hiding this 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.
src/librustdoc/clean/types.rs
Outdated
#[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, | ||
} | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice!
src/librustdoc/clean/types.rs
Outdated
/// 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok!
552fd86
to
6b6004f
Compare
Done! |
… type declaration
6b6004f
to
1c23349
Compare
Done as well. |
Thanks! @bors r+ rollup=iffy (iffy because it has some potential for merge conflicts) |
📌 Commit 1c23349 has been approved by |
☀️ Test successful - checks-actions |
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 |
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