[PATCH] D21785: [RFC]Add unittests to AArch64TargetParser

[jojo.ma via llvm-commits]

jojo added inline comments.

================
Comment at: include/llvm/Support/TargetParser.h:152
@@ +151,3 @@
+#include "AArch64TargetParser.def"
+  AAK_LAST
+};
----------------
compnerd wrote:
> I think that you should use the `AK_` prefix as is the norm.  That said, you could do something else which IMO is better: use `enum class` and drop the prefix.
If we use ##enum class##, we must do type casting when we perform operations between "unsigned int" and archkind. There are a lot of cases like this, in the TargetParser and places using it. And some generic structure definition shared between arm and aarch64, should be made  a set for each.
Do we have to do this? Let AK_ point to ARM, and AAK_ point to AArch64 seems to make sense also.  

================
Comment at: unittests/Support/TargetParserTest.cpp:24
@@ -21,3 +23,3 @@
 #undef AARCH64_ARCH
 };
 
----------------
compnerd wrote:
> Please delete this.  I had added it to avoid hardcoding the offset.  Just iterate over the enumeration below.  In fact, it is unused.
Indeed,remove it.

================
Comment at: unittests/Support/TargetParserTest.cpp:68
@@ +67,3 @@
+#undef AARCH64_CPU_NAME
+};
+
----------------
compnerd wrote:
> This set really feels weird to me.
I removed two of these which are unnecessary.
The remaining two arrays are needed for some test items,with the help of def file can avoid writing a long array.And if there are changes happened on arch or cpu in def,we can do as small
changes as possible.
Hi,compnerd,If you still feel it can not be accepted,Could you say it further?  Can't you accept the way they are defined? Or just don't want to have something like that?  so it can get me more clear.
Thank you very much! :) 




http://reviews.llvm.org/D21785