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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith requested changes to this revision.
dexonsmith added inline comments.
This revision now requires changes to proceed.


================
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>,
----------------
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).


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:293
 static T mergeForwardValue(T KeyPath, U Value) {
-  return Value;
+  return static_cast<T>(Value);
 }
----------------
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));
}
```


================
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))
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83979/new/

https://reviews.llvm.org/D83979