[llvm-commits] [PATCH] ARM build attribute reading support

[Renato Golin]

Hi Amara,

This is a great patch, but you need more tests, I believe. At least
one for each field you add, but possibly more.

I also have a few questions:

+  if (getArch() != Triple::arm)
+    report_fatal_error("Cannot read ARM build attributes of a non-ARM file.");

Triple::thumb is not supported?

If so, shouldn't we check, or at least warn, for ISA compatibility?


+  // Legal Values for CPU_arch, (=6), uleb128
+  enum CPUArch {

Does big.LITTLE have its own value, here?


+  // The following have a lot of common use cases
+  enum {
+    //ARMISAUse (=8), uleb128  and THUMBISAUse (=9), uleb128
+    Not_Allowed = 0,
+    Allowed = 1,
+
+    // FP_arch (=10), uleb128 (formerly Tag_VFP_arch = 10)
+    AllowFPv2  = 2, // v2 FP ISA permitted (implies use of the v1 FP ISA)

(...)

I'm in two minds about this. I know it doesn't matter much, but I
think multiple anonymous enums would be clearer.


+  class ARMGenericBuildAttrInfo {
+public:
+    ARMGenericBuildAttrInfo()
+      : Tag_CPU_arch(v4T),
+        Tag_CPU_arch_profile(Not_Applicable),
(...)
+private:
+    CPUArch Tag_CPU_arch;
+    CPUArchProfile Tag_CPU_arch_profile;

You seem to have many private fields, but public getters and setters,
which is the same as POD struct. Why not use POD directly?


The rest looks good.

cheers,
--renato