[PATCH] D26968: [ARM] Add Driver support for emitting the missing Tag_ABI_enum_size build attribute values

[Renato Golin via cfe-commits]

rengolin added a comment.

Hi Alexandros,

My interpretation of `Tag_ABI_enum_size` is that value 3 is that **all** enums are 32-bit and value 4 is that only those ABI-visible (ie. public interfaces) have to be 32-bits. This is similar to the other PCS tags.

So, the table is:

- 0: no enums
- 1: short enums
- 2: all 32-bit
- 3: public 32-bit

With the default being 2 in GCC.

My reading of your code here is that you assume short|abi = short, which doesn't match the table in the ABI.



================
Comment at: lib/Basic/Targets.cpp:5407
+      Builder.defineMacro("__ARM_SIZEOF_MINIMAL_ENUM",
+                          Opts.ShortEnums || Opts.ABIEnums ? "1" : "4");
 
----------------
Isn't ABIEnums 4? Shouldn't this be:

    Opts.ShortEnums || !Opts.ABIEnums

Is it even valid to have ABIEnums && ShortEnums at the same time?


================
Comment at: lib/Driver/Tools.cpp:5731
+
+  // The last of -fno-enums, -fshort-enums, -fabi-enums wins.
+  Arg *Enum;
----------------
If this is true, why go the extra complexity of mapping all possible states? Why not just use one line:

    Enum = Args.getLastArg(options::OPT_fno_enums, options::OPT_fshort_enums, options::OPT_fabi_enums);

and be done with it? Short and ABI are not compatible, it's either one or the other.


================
Comment at: lib/Driver/Tools.cpp:5741
+  else
+    Enum = Args.getLastArg(options::OPT_fno_enums);
+  if (Enum) {
----------------
This code is confusing and merging with each other. Please add some empty lines between them.


https://reviews.llvm.org/D26968