[PATCH] D96280: [clang][cli] Round-trip the whole CompilerInvocation

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

Updated the patch, removed `[WIP]` from the title and replied to comments.



================
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;
----------------
dexonsmith wrote:
> Was it intentional to comment this out? If so, probably better to delete the line of code.
Yes, it was intentional. I've now replaced the commented-out code with a comment explaining why profile list arguments are not generated here.


================
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);
 
----------------
dexonsmith wrote:
> Was it intentional to comment out this code? If so, probably better to delete it.
Same as above.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96280