jansvoboda11 added a comment.
Thanks for catching these!
================
Comment at: clang/include/clang/Driver/Options.td:1292-1303
def fdwarf_exceptions : Flag<["-"], "fdwarf-exceptions">, Group<f_Group>,
- Flags<[CC1Option]>, HelpText<"Use DWARF style exceptions">;
+ Flags<[CC1Option]>, HelpText<"Use DWARF style exceptions">,
+ MarshallingInfoFlag<"LangOpts->DWARFExceptions">;
def fsjlj_exceptions : Flag<["-"], "fsjlj-exceptions">, Group<f_Group>,
- Flags<[CC1Option]>, HelpText<"Use SjLj style exceptions">;
+ Flags<[CC1Option]>, HelpText<"Use SjLj style exceptions">,
+ MarshallingInfoFlag<"LangOpts->SjLjExceptions">;
def fseh_exceptions : Flag<["-"], "fseh-exceptions">, Group<f_Group>,
----------------
dexonsmith wrote:
> These options should be mutually exclusive -- as in, the last flag wins --
> but I don't see how that's implemented now (the previous logic was via
> `getLastArg`). If that is working properly, can you explain how?
>
> If it's not working right now, my suggestion would be to separate these
> options out to do in a separate patch series. I would suggest, rather than
> modelling the current behaviour, we leverage our flexibility to change `-cc1`
> options (e.g., could do three patches, where the first adds accessors to
> LangOpts and updates all users, the second changes the keypath to a single
> `ExceptionStyle` enum, and then the third patch changes the `-cc1` option to
> `-fexception-style` and starts using the marshalling infrastructure).
Nice catch, thanks! I'll revert these changes and tweak the `-cc1` command-line
in a follow-up.
Another option would be to keep these changes and check the exclusivity in
`FixupInvocation`, but I prefer the enum.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:293
static T mergeForwardValue(T KeyPath, U Value) {
- return Value;
+ return static_cast<T>(Value);
}
----------------
dexonsmith wrote:
> These nits might be better to do in a follow-up, which also updated
> `extractForwardValue`, but since I'm seeing it now:
> - Should this use `std::move`?
> - Can we drop the `KeyPath` name?
> ```
> template <typename T, typename U>
> static T mergeForwardValue(T /*KeyPath*/, U Value) {
> return static_cast<T>(std::move(Value));
> }
> ```
Adding `std::move` here and in `extractForwardValue` makes sense to me, I can
do that in a follow-up.
May I ask why are you so keen on dropping names of unused parameters?
To me, commenting the name out seems to only add unnecessary syntax, and
dropping it entirely makes the signature less clear.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3655-3666
llvm::Triple T(Res.getTargetOpts().Triple);
if (DashX.getFormat() == InputKind::Precompiled ||
DashX.getLanguage() == Language::LLVM_IR) {
- // ObjCAAutoRefCount and Sanitize LangOpts are used to setup the
- // PassManager in BackendUtil.cpp. They need to be initializd no matter
- // what the input type is.
- if (Args.hasArg(OPT_fobjc_arc))
----------------
dexonsmith wrote:
> Previously, these arguments were only claimed depending on `-x`; are we
> changing to claim these all the time? If so, that should be considered
> separately; I think in general we may want the ability to do claim some
> options only conditionally; @Bigcheese , any thoughts here?
`LangOpts.PIE` is unconditionally populated at line `2901`, so I think removing
it here is fine.
You're right about `LangOpts.ObjCAutoRefCount`, though. I think keeping the
semantics the same is preferable, even though all tests pass even after this
change. (It has also been removed at line `2547` which also doesn't seem
right.) I'll revert this for now and come back to it when we land
`ShouldParseIf` in D84674.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83979/new/
https://reviews.llvm.org/D83979
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
