Conversation
Changes that make this macro easier to read.
- `DepKindDefs` is an enum that is used purely to generate a list of
ascending numbers at compile time, to implement the functions within
`dep_kinds`. This commit replaces it with a `${index()}` macro
metavar. (The comment about u16 data representation doesn't seem
relevant any more.)
- `DEP_KIND_VARIANTS` just counts the number of dep kind variants. It's
implemented via `len()` and a const loop to ensure `DepKindDefs` truly
implements a list of ascending numbers. This commit replaces it with a
`${count(..)}` macro metavar. This commit also renames it to make
clearer it's a count of varants, not an actual list/array/vec of
variants.
- This commit uses `stringify!` more for variant-names-as-strings,
instead of going indirectly via `label_strs`.
rustbot
added
S-waiting-on-review
| pub(super) fn dep_kind_from_label_string(label: &str) -> Result<DepKind, ()> { | ||
| match label { | ||
| $( self::label_strs::$variant => Ok(self::dep_kinds::$variant), )* | ||
| $( stringify!($variant) => Ok(self::dep_kinds::$variant), )* |
There was a problem hiding this comment.
At the time I was thinking that it would be good to reduce the amount of stringify!($name), but I'm not actually very confident about that being the right direction.
So with hindsight, I don't really object to changing it back.
There was a problem hiding this comment.
For me, mentally expanding stringify!($variant) is easy, while working out what self::label_strs::$variant means is harder because I have to look further down.
|
For some historical context, the split between I don't know if that has actionable consequences for this PR, but it seems worth bearing in mind as we continue to dismantle |
| pub(crate) const DEP_KIND_VARIANTS: u16 = { | ||
| let deps = &[$(dep_kinds::$variant,)*]; | ||
| let mut i = 0; | ||
| while i < deps.len() { | ||
| if i != deps[i].as_usize() { | ||
| panic!(); | ||
| } | ||
| i += 1; | ||
| } | ||
| deps.len() as u16 | ||
| }; |
There was a problem hiding this comment.
Do we have any other check that the numeric values continue to be dense and ascending from 0?
That's currently still true in the new implementation, but I don't know if there's anything ensuring that it remains true.
There was a problem hiding this comment.
The numeric values come from the macro metavar ${index()}, which is "The current index of the inner-most repetition.". I.e. it's just a 0..n counter.
AFAICT this sanity check was overkill for the enum case, because enums are (I think?) guaranteed to be numbered from 0..n but I can see why someone might be uncertain about that and want to check it. But checking that ${index()} actually implements 0..n seems paranoid, that's fully in "I don't even trust the compiler" territory.
There was a problem hiding this comment.
This check seems awfully familiar, as if I wrote it. If I did, I was unsure at the time if it was justified, and the cost seemed acceptable at the time. Fine with me if it's now too clunky.
There was a problem hiding this comment.
My concern is not whether ${index()} implements 0..n, but rather that if someone tries to modify this code without knowing that the 0..n is load-bearing, then they'll end up with either mysterious crashes when running the compiler, or (worst-case) very subtle incremental-compilation bugs.
There was a problem hiding this comment.
All that said, after #152516 has landed I question whether DepKind needs to be a u16 newtype at all.
My understanding is that #115920 split off DepKind from the enum, because the enum was in rustc_middle but DepKind wanted to be upstream in rustc_query_system.
If DepKind is moved back to rustc_middle, we should be able to just make it an enum again.
There was a problem hiding this comment.
This check seems awfully familiar, as if I wrote it.
- Looks like you did: Encode DepKind as u16 #115391
|
☔ The latest upstream changes (presumably #152574) made this pull request unmergeable. Please resolve the merge conflicts. |
5 participants
Changes that make this macro easier to read.
DepKindDefsis an enum that is used purely to generate a list of ascending numbers at compile time, to implement the functions withindep_kinds. This commit replaces it with a${index()}macro metavar. (The comment about u16 data representation doesn't seem relevant any more.)DEP_KIND_VARIANTSjust counts the number of dep kind variants. It's implemented vialen()and a const loop to ensureDepKindDefstruly implements a list of ascending numbers. This commit replaces it with a${count(..)}macro metavar. This commit also renames it to make clearer it's a count of varants, not an actual list/array/vec of variants.This commit uses
stringify!more for variant-names-as-strings, instead of going indirectly vialabel_strs.r? @saethlin