Switch GetExprCategory to be table-driven.
#6371
Conversation
Avoid using a large switch that needs to be manually extended when adding a new kind of instruction. Instead, the expression category for an instruction is now specified when defining the `InstKind`.
danakj
left a comment
danakj
left a comment
There was a problem hiding this comment.
This is very nice, thanks. I have one suggestion below.
| // The kind of category for this instruction. This includes all values in | ||
| // `ExprCategory`, for the case where the category of the instruction only | ||
| // depedns on its kind, plus some additional kinds. | ||
| enum Kind : int8_t { |
There was a problem hiding this comment.
Consider adding [[clang::enum_extensibility(open)]] to help explain this enum holds other values too?
Alternatively, consider just using a uint8_t as the storage for the kind_, then casting to either ExprCategory or Kind. It felt a little weird to conceptually say Kind contains all the values of ExprCategory but then the return type of AsInstDependentKind is also Kind when we want to exclude ExprCategory values.
If that sounded good, maybe also consider moving Kind to be DependentExprCategory as an enum outside of this class, a peer of ExprCategory? I found it a bit unclear to see some instructions set ExprCategory::X and others InstExprCategory::Y, as the Inst prefix isn't really differentiating things.
2 participants
Avoid using a large switch that needs to be manually extended when adding a new kind of instruction. Instead, the expression category for an instruction is now specified when defining the
InstKind.In passing, add a distinct expression category value for patterns. This isn't used for much except some error checking at the moment, but it keeps the number of instructions that we need to manually classify as
NotExprdespite having a type very low.