[PATCH] D21785: [RFC]Add unittests to {ARM | AArch64}TargetParser

[Saleem Abdulrasool via llvm-commits]

compnerd added a comment.

There are a bunch of place that could use a range based for loop, please prefer them instead.


================
Comment at: lib/Support/TargetParser.cpp:419
@@ -418,3 +418,3 @@
 #define AARCH64_CPU_NAME(NAME, ID, DEFAULT_FPU, IS_DEFAULT, DEFAULT_EXT) \
-    .Case(NAME, AArch64ARCHNames[ID].ArchBaseExtensions | DEFAULT_EXT)
+    .Case(NAME, DEFAULT_EXT)
 #include "llvm/Support/AArch64TargetParser.def"
----------------
Isn't this the original bug that is being fixed rather than just adding tests for the target parser?

================
Comment at: lib/Support/TargetParser.cpp:455
@@ -454,2 +454,3 @@
                                      std::vector<const char *> &Features) {
-  if (ArchKind == ARM::AK_INVALID || ArchKind >= ARM::AK_LAST)
+  if (ArchKind == static_cast<unsigned>(AArch64::ArchKind::AK_INVALID) ||
+      ArchKind >= static_cast<unsigned>(AArch64::ArchKind::AK_LAST))
----------------
rengolin wrote:
> Now I see why you wanted a different enum.
> 
> @compnerd, I really don't mind either way, so I'll leave it up to you. :)
I don't think that this is necessarily bad.  It does make it more obvious if you are targeting a newer architecture variant.

================
Comment at: lib/Support/TargetParser.cpp:463
@@ -461,3 +462,3 @@
     Features.push_back("+v8.2a");
 
   return true;
----------------
Given that you are doing explicit checks, why not simplify this to:

    if (ArchKind == ...)
      ...
    if (ArchKind == ...)
      ...
    return ArchKind > AArch64::ArchKind::AK_INVALID && ArchKind < AArch64::ArchKind::AK_LAST;

================
Comment at: lib/Support/TargetParser.cpp:468
@@ -466,5 +467,3 @@
 StringRef llvm::AArch64::getArchName(unsigned ArchKind) {
-  for (const auto &AI : AArch64ARCHNames)
-    if (AI.ID == ArchKind)
-      return AI.getName();
-  return StringRef();
+  if (ArchKind >= static_cast<unsigned>(AArch64::ArchKind::AK_LAST))
+    return StringRef();
----------------
Shouldn't this check for `AK_INVALID` as well?

================
Comment at: lib/Support/TargetParser.cpp:488
@@ -487,5 +487,3 @@
 unsigned llvm::AArch64::getArchAttr(unsigned ArchKind) {
-  for (const auto &AI : AArch64ARCHNames)
-    if (AI.ID == ArchKind)
-      return AI.ArchAttr;
-  return ARMBuildAttrs::CPUArch::v8_A;
+  if (ArchKind >= static_cast<unsigned>(AArch64::ArchKind::AK_LAST))
+    return ARMBuildAttrs::CPUArch::v8_A;
----------------
Shouldn't this check for `AK_INVALID` as well?

================
Comment at: unittests/Support/TargetParserTest.cpp:180
@@ +179,3 @@
+
+  for (unsigned i = 0; i< array_lengthof(kARMCPUNames); i++)
+    EXPECT_EQ(kARMCPUNames[i].DefaultFPU,
----------------
space before the <.

Why not use a range based loop?

    for (const auto &ARMCPUName : kARMCPUNames)
      EXPECT_EQ(ARMCPUName.DefaultFPU, ARM::getDefaultFPU(ARMCPUName.Name, 0));

================
Comment at: unittests/Support/TargetParserTest.cpp:192
@@ +191,3 @@
+
+  for (unsigned i = 0; i< array_lengthof(kARMCPUNames); i++) {
+    unsigned ArchKind = kARMCPUNames[i].ID;
----------------
Similar.

================
Comment at: unittests/Support/TargetParserTest.cpp:263
@@ +262,3 @@
+    { "xscale", "noxscale", nullptr, nullptr}
+  };
+
----------------
clang-format this


https://reviews.llvm.org/D21785