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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
