rustc_target: Begin unifying Arch enum #142775
Conversation
rustbot
added
O-apple
|
These commits modify compiler targets. |
|
also cc @thomcc I intend to make the QNX targets part of this PR too, and also reuse this code elsewhere in |
|
☔ The latest upstream changes (presumably #145126) made this pull request unmergeable. Please resolve the merge conflicts. |
| pub(crate) enum Arch { | ||
| Arm(ArmVer), | ||
| Aarch64(Aarch64Ver), | ||
| X86_32(Ix86), | ||
| X86_64(X86_64Ver), | ||
| } |
There was a problem hiding this comment.
Hmm, I get why you'd want to simplify things in this manner (arch enum with subarch enums), but I'm not sure that's actually a good idea?
I particularly don't like the Arch::Arm64 to Aarch64Ver::V8A.into() change, it makes it harder to understand what's going on (are we specifying the arch or subarch here?), and it becomes incorrect as soon as you pass -Ctarget-cpu=apple-a10.
I'd prefer a single-level Arch enum, and rather have helpers like .is_aarch64() or .is_arm() (if those are even useful in the face of things like ARM64EC).
rustbot
added
S-waiting-on-author
|
It's always been my experience that "single enum with helpers" is
worse on a few different levels.
Namely, my goal is generally to incentivize matching against precisely
the relevant patterns and to minimize the noise of other patterns. In
particular, I believe the top level `match` should ideally not bottom
out to a plain `_`.
The problem is that such is effectively what `if let` does.
If the only complaint is an `into`, I can work on reducing that. But
if the problems are deeper, I am happy to think of a different
approach if this does not suit.
It's been a while so I'm not sure what you mean by
`-Ctarget-cpu=apple-a10` changing things, though. Do the number of
changes necessary actually alter significantly...?
|
There are a few different instances of this Arch enum in
rustc_target. I would like to unify them so that we use shared code in more cases, as part of an incremental step towards turning Target into a proper builder with type-level correctness instead of its current constructor. However, it's definitely true the immediate changes look somewhat jank and have the disadvantage of in some cases handling some code less exhaustively, so I'm going to ask some maintainers if it's okay.This PR currently starts with lifting from the Apple code, so
r? @madsmtm