[PATCH] D26968: [ARM] Add Driver support for emitting the missing Tag_ABI_enum_size build attribute values
Renato Golin via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 22 06:08:09 PST 2016
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
More information about the cfe-commits
mailing list