[PATCH] D35882: [TargetParser] Use enum classes for various ARM kind enums.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Support/TargetParser.cpp:380
-  unsigned AK = parseArch(Arch);
-  if (AK == ARM::AK_INVALID)
-    return StringRef();
----------------
rovka wrote:
> This can still return INVALID, I don't think you should remove the check just yet.
I thought a change in the .def file made that unnecessary, but I was mistaken.


================
Comment at: lib/Support/TargetParser.cpp:766
     return 8;
+  default:
+    return 0;
----------------
rovka wrote:
> I'd rather make the switch fully covered than add a default. Also, you could add an unreachable before the end of the function.
Excellent point! I've removed the default case from this switch and the switch above in ARM::parseArchProfile.


================
Comment at: unittests/Support/TargetParserTest.cpp:452
        FK = static_cast<ARM::FPUKind>(static_cast<unsigned>(FK) + 1))
-    if (FK == ARM::FK_LAST)
-      EXPECT_EQ(0U, ARM::getFPUVersion(FK));
+    if (FK == ARM::FK_LAST || ARM::getFPUName(FK) == "invalid" ||
+        ARM::getFPUName(FK) == "none" || ARM::getFPUName(FK) == "softvfp")
----------------
rovka wrote:
> Are the extra checks necessary?
Unfortunately yes, but I am not sure if that test adds much value. 

It seems like the previous test just checked if "0 <= ARM::getFPUVersion(FK)" for all FK holds. I think the intention was to check that it always returns an element in the enum, but wouldn't the condition be always true, because ARM::getFPUVersion(FK) used to return unsigned?

I think the tests in ARMFPUNeonSupportLevel and ARMFPURestriction have the same problem.


================
Comment at: unittests/Support/TargetParserTest.cpp:790
+  for (unsigned i = 0;
+       i <= static_cast<unsigned>(AArch64::ArchKind::ARMV8_2A);
+       i++) {
----------------
rovka wrote:
> This kind of iteration was sort of ok when there was an AK_LAST, but I think with that gone there's the chance that this test won't get updated when someone adds something after ARMV8_2A.
> 
> One alternative would be to add the enum values to a vector (you can just #define AARCH64_ARCH(NAME, ID [...]) to push_back and include AArch64TargetParser.def) and iterate over the vector instead.
Agreed, thanks for the vector tip! :)


https://reviews.llvm.org/D35882