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

[Diana Picus via Phabricator via llvm-commits]

rovka accepted this revision.
rovka added a comment.
This revision is now accepted and ready to land.

LGTM.



================
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")
----------------
fhahn wrote:
> 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.
You're right, these tests were a bit iffy to begin with.


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


https://reviews.llvm.org/D35882