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

[Renato Golin via cfe-commits]

rengolin added inline comments.


================
Comment at: lib/Basic/Targets.cpp:5407
+      Builder.defineMacro("__ARM_SIZEOF_MINIMAL_ENUM",
+                          Opts.ShortEnums || Opts.ABIEnums ? "1" : "4");
 
----------------
labrinea wrote:
> rengolin wrote:
> > Isn't ABIEnums 4? Shouldn't this be:
> > 
> >     Opts.ShortEnums || !Opts.ABIEnums
> > 
> > Is it even valid to have ABIEnums && ShortEnums at the same time?
> My understanding is that ABIEnums requires 32-bit enums across an ABI-complying interface, but allows short enums outside of it. Therefore __ARM_SIZEOF_MINIMAL_ENUM could be any of 1 or 4.
> 
> ABIEnums && ShortEnums cannot be both set at the same time.
If all four options are mutually exclusive, than they should be a single integer option, not multiple boolean ones.


================
Comment at: lib/Driver/Tools.cpp:5731
+
+  // The last of -fno-enums, -fshort-enums, -fabi-enums wins.
+  Arg *Enum;
----------------
labrinea wrote:
> rengolin wrote:
> > 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.
> I don't like this complicated logic either but it can handle cases like:
> -fshort-enums -fno-enums -fabi-enums -fno-abi-enums
> where -fno-enums should win. I've added a test for this sequence (test/Driver/clang_f_opts.c, line 459)
> The suggested logic would ignore '-fno-abi-enums', which is the last argument. If we are happy with rejecting all the preceding flags and keeping just the last one this would work:
> ```
> Args.getLastArg(options::OPT_fno_enums, options::OPT_fshort_enums, options::OPT_fno_short_enums,
>                 options::OPT_fabi_enums, options::OPT_fno_abi_enums);
> ```
> Alternatively an equally ugly logic that handles complicated sequences is:
> ```
> if (Arg *Enum = Args.getLastArg(options::OPT_fno_enums,
>              ShortEnums ? options::OPT_fshort_enums : options::OPT_INVALID,
>                ABIEnums ? options::OPT_fabi_enums   : options::OPT_INVALID)) {
> ```
I disagree. These are different flags controlling the same thing, so the last-option wins.

How does -ffast-math vs its internal options are handled? How is -O handled? Or the other ABI flags like vfp, hard-float?

Easier still, have a look at unsigned/signed char below. Last arg wins.


https://reviews.llvm.org/D26968