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

[Diana Picus via Phabricator via llvm-commits]

rovka added inline comments.


================
Comment at: lib/Support/TargetParser.cpp:380
-  unsigned AK = parseArch(Arch);
-  if (AK == ARM::AK_INVALID)
-    return StringRef();
----------------
This can still return INVALID, I don't think you should remove the check just yet.


================
Comment at: lib/Support/TargetParser.cpp:766
     return 8;
+  default:
+    return 0;
----------------
I'd rather make the switch fully covered than add a default. Also, you could add an unreachable before the end of the function.


================
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")
----------------
Are the extra checks necessary?


================
Comment at: unittests/Support/TargetParserTest.cpp:790
+  for (unsigned i = 0;
+       i <= static_cast<unsigned>(AArch64::ArchKind::ARMV8_2A);
+       i++) {
----------------
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.


https://reviews.llvm.org/D35882