dexonsmith added a comment.
Mostly looks great; happy to see this come full circle and remove the
round-tripping boilerplate. Just a couple of questions inline below.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1940-1941
case EDK_ProfileList:
- GenerateArg(Args, OPT_fprofile_list_EQ, Dep.first, SA);
+ // Generated from LanguageOptions.
+ // GenerateArg(Args, OPT_fprofile_list_EQ, Dep.first, SA);
break;
----------------
Was it intentional to comment this out? If so, probably better to delete the
line of code.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3438-3439
- if (Opts.NoInlineDefine && Opts.Optimize)
- GenerateArg(Args, OPT_fno_inline, SA);
+ // if (Opts.NoInlineDefine && Opts.Optimize)
+ // GenerateArg(Args, OPT_fno_inline, SA);
----------------
Was it intentional to comment out this code? If so, probably better to delete
it.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4534-4547
+ InputKind DashX = FrontendOpts.DashX;
+ if (DashX.getFormat() == InputKind::Precompiled ||
+ DashX.getLanguage() == Language::LLVM_IR) {
+ if (LangOpts->ObjCAutoRefCount)
+ GenerateArg(Args, OPT_fobjc_arc, SA);
+ if (LangOpts->PICLevel != 0)
+ GenerateArg(Args, OPT_pic_level, Twine(LangOpts->PICLevel), SA);
----------------
This change, like the commented out code I already pointed at, seems not
strictly-related to changing round-tripping to happen at a higher level. Is
there some reason it's tied to landing at the same time? (If that somehow
avoids a bunch of refactoring that'll have to be undone after, fine by me, but
otherwise it'd be nice to make this particular patch as clean as possible I
think and land the more functional changes ahead of time.)
I also wonder if this special-case logic could/should be buried inside
`GenerateLangArgs`, possibly by passing in `DashX` as an argument. (Maybe with
a FIXME to handle via ShouldParseIf? E.g., maybe almost all lang options could
be moved to a `let` scope that turns on ShouldParseIf with this logic, and
these four are left outside of it. Up to you, whether you think that's a good
idea.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96280/new/
https://reviews.llvm.org/D96280
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
