Cleanup codegen traits #130457
Merged
Cleanup codegen traits #130457
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It only needs `Self::Value` and `Self::Type`, so it can be a subtrait of `BackendTypes`. That is a smaller and simpler trait than `HasCodegen` (which includes `BackendTypes` and a lot more).
It has `Backend` and `Deref` boudns, plus an associated type `CodegenCx`, and it has a single use. This commit "inlines" it into `BuilderMethods`, which makes the complicated backend trait situation a little simpler.
It's a trait that aggregates five other traits. But consider the places that use it. - `BuilderMethods`: requires three of the five traits. - `CodegenMethods`: requires zero(!) of the five traits. - `BaseTypeMethods`: requires two of the five traits. - `LayoutTypeMethods`: requires three of the five traits. - `TypeMembershipMethods`: requires one of the five traits. This commit just removes it, which makes everything simpler.
They both are part of `BuilderMethods`, and so should have `Builder` in their name like all the other traits in `BuilderMethods`.
Some of these are pulled in indirectly, e.g. `MiscMethods` via `TypeMethods`.
Specifically, put them where they are genuinely required, i.e. the outermost place they can be.
Supertraits of `BuilderMethods` are all called `XyzBuilderMethods`. Supertraits of `CodegenMethods` are all called `XyzMethods`. This commit changes the latter to `XyzCodegenMethods`, for consistency.
This avoids some repetitive boilerplate code.
rustbot
added
S-waiting-on-review
|
Some opinionated changes here, let's see how it goes. Best reviewed one commit at a time. |
bjorn3
reviewed
Sep 17, 2024
|
I'm not entirely happy with "Rename supertraits of CodegenMethods." Other than that everything looks like an improvement to me. |
I can remove it, though I would be interested to hear why you don't like it. The |
|
I guess it is fine to keep that commit. @bors r+ |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 18, 2024
…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#130457 (Cleanup codegen traits) - rust-lang#130471 (Add zlib to musl dist image so rust-lld will support zlib compression for debug info there.) - rust-lang#130507 (Improve handling of raw-idents in check-cfg) - rust-lang#130509 (llvm-wrapper: adapt for LLVM API changes, second try) - rust-lang#130510 (doc: the source of `LetStmt` can also be `AssignDesugar`) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 18, 2024
Rollup merge of rust-lang#130457 - nnethercote:cleanup-codegen-traits, r=bjorn3 Cleanup codegen traits The traits governing codegen are quite complicated and hard to follow. This PR cleans them up a bit. r? `@bjorn3`
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The traits governing codegen are quite complicated and hard to follow. This PR cleans them up a bit.
r? @bjorn3