dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.
LGTM! One more more comment inline.
================
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);
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > 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.)
> This change is necessary for the patch.
>
> Until now, `GenerateLangArgs` was called (during round-trip) from
> `ParseLangArgs`, which is behind the same condition in `CreateFromArgs`: `if
> (!(DashX.getFormat() == InputKind::Precompiled || DashX.getLanguage() ==
> Language::LLVM_IR))`. This change attempts to preserve the previous behavior
> and also to avoid calling `GenerateLangArgs` with unexpected/invalid `DashX`.
>
> I agree eventually moving the logic into `{Parse,Generate}LangArgs` would be
> nice, but I'd like to do that in a follow up patch.
That makes sense; please add a FIXME to that effect.
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
