labrinea added inline comments.
================
Comment at: lib/Basic/Targets.cpp:5407
+ Builder.defineMacro("__ARM_SIZEOF_MINIMAL_ENUM",
+ Opts.ShortEnums || Opts.ABIEnums ? "1" : "4");
----------------
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.
================
Comment at: lib/Driver/Tools.cpp:5731
+
+ // The last of -fno-enums, -fshort-enums, -fabi-enums wins.
+ Arg *Enum;
----------------
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)) {
```
https://reviews.llvm.org/D26968
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
