Clean up clean/types.rs file by making the implementations follow the type declaration #89203
Conversation
rust-highfive
added
the
S-waiting-on-review
GuillaumeGomez
added
C-cleanup
camelid
added
the
S-waiting-on-review
There was a problem hiding this comment.
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.
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.
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.
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.
camelid
added
S-waiting-on-author
GuillaumeGomez
force-pushed
the
cleanup-rustdoc-types
branch
from
552fd86 to
6b6004f
Compare
GuillaumeGomez
added
S-waiting-on-review
|
Done! |
… type declaration
GuillaumeGomez
force-pushed
the
cleanup-rustdoc-types
branch
from
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 |
bors
added
S-waiting-on-bors
|
☀️ 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