[PATCH] D83979: [clang][cli] Port LangOpts option flags to new option parsing system

[Jan Svoboda via Phabricator via cfe-commits]

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