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

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

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

This LGTM now.



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:293
 static T mergeForwardValue(T KeyPath, U Value) {
-  return Value;
+  return static_cast<T>(Value);
 }
----------------
jansvoboda11 wrote:
> 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.
Dropping unused parameter names makes it clear to the reader that it's intentional that the parameter is unused (for longer functions, it can also clarify that it's not used).

If there were a forward declaration for this, I think it would make sense to have the parameter name in the forward declaration (to document what it's for) and drop it entirely in the definition (to document that it's skipped intentionally), but since there's no forward declaration I think commenting it out is a good compromise.


================
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))
----------------
jansvoboda11 wrote:
> 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.
Okay, that sounds good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83979