[PATCH] D22956: Ajust two tests implementation of TargetParserTest

[Renato Golin via llvm-commits]

rengolin added a comment.

In https://reviews.llvm.org/D22956#501906, @jojo wrote:

> 1. Just  iterate to AEK_RAS, as shown in this diff. (smallest change) As AEK_RAS in the enum is the last one supported in the current design,so iterate to it may seems odd but is enough.


It is odd, and error prone, if we re-order or change things.

> 2. Add option AEK_LAST to enum ArchExtKind,then iterate to AEK_LAST.(biggest change, I prefer this one)


Me too. But I'm still not convinced the two additional tests (ARMArchExtName, ARMHWDivName) add anything of value.

If we make a mistake in the enum, the test will still pass, since both the enum and the test array are generated from the same source (the .def file).

We're already testing the extensions in ARMArchExtFeature, so why not just leave it at that?

cheers,
--renato


================
Comment at: include/llvm/Support/TargetParser.h:93
@@ -92,2 +92,3 @@
   AEK_XSCALE = 0x80000000,
+  AEK_LAST = 0x2000
 };
----------------
AEK_LAST seems like a bad name for this... Maybe AEK_LAST_SUPPORTED?

Also, best if put in order, ie. after AEK_RAS.

================
Comment at: lib/Support/TargetParser.cpp:205
@@ -204,3 +204,3 @@
 
-  if (HWDivKind == ARM::AEK_INVALID)
+  if (HWDivKind >= ARM::AEK_LAST || HWDivKind == ARM::AEK_INVALID)
     return false;
----------------
This encodes the wrong information. This is telling you that *no* unsupported arch has HDIV, when this might not be true.

Why do you need to change this at all?


https://reviews.llvm.org/D22956