[PATCH] D27337: [globalisel][aarch64] Fix unintended assumptions about PartialMappingIdx. NFC.

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.cpp:118
     const PartialMapping &Map =                                                \
-        AArch64::PartMappings[AArch64::PartialMappingIdx::Idx];                \
+        AArch64::PartMappings[AArch64::Idx - AArch64::PartialMappingIdx_Min];  \
     (void) Map;                                                                \
----------------
qcolombet wrote:
> I'd keep `::PartialMappingIdx::`, I like being explicit about the type we use (and that shouldn't be redundant for the _Min part when you'd use a shorter name, see the other review).
Ok. Would you like me to make the explicit namespace mandatory by changing the type to an 'enum class'? This would also fix the prefix issue I mentioned on the other review.


================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.cpp:135
   do {                                                                         \
-    AArch64::PartialMappingIdx PartialMapBaseIdx =                             \
-        AArch64::PartialMappingIdx::RBName##Size;                              \
+    unsigned PartialMapBaseIdx =                                               \
+        AArch64::RBName##Size - AArch64::PartialMappingIdx_Min;                \
----------------
qcolombet wrote:
> Could we keep the PartialMappingIdx type instead of unsigned?
The compiler warns about using PartialMappingIdx because it's not a PartialMappingIdx after the subtraction. The warning could be silenced with a cast like so:
  PartialMappingIdx PartialMapBaseIdx = (PartialMappingIdx)(AArch64::PartialMappingIdx::RBName##Size - AArch64::PartialMappingIdx::PartialMappingIdx_Min);
but this is still misleading because the values don't match up with the enumerators.


https://reviews.llvm.org/D27337