[PATCH] D86999: getClangStripDependencyFileAdjuster(): Do not remove -M args when using MSVC cl driver

[Shivanshu Goyal via Phabricator via llvm-commits]

shivanshu3 marked 2 inline comments as done.
shivanshu3 added inline comments.


================
Comment at: clang/lib/Tooling/ArgumentsAdjusters.cpp:117
+      // do not remove those when using the cl driver.
+      bool IsDependencyFileArg;
+      if (Arg.startswith("/showIncludes") || Arg.startswith("-showIncludes"))
----------------
hans wrote:
> shivanshu3 wrote:
> > hans wrote:
> > > Instead of a bool and if below, I'd suggest if-statements and early continues instead. Breaking up the old if-statement is nice though, and maybe the comment from above about what flags to ignore could be moved down here to those if statements. For example:
> > > 
> > > 
> > > ```
> > > // -M flags blah blah
> > > if (Arg.startswith("-M") && !UsingClDriver)
> > >   continue;
> > > // MSVC flags blah blah
> > > if (Arg.startswith("/showIncludes") || Arg.startswith("-showIncludes"))
> > >   continue;
> > > AdjustedArgs.push_back(Args[i]);
> > > ```
> > I realized that with my original change, we would skip the next argument under cl driver mode when -MT was used, which would be wrong. The next argument should only be skipped when `IsDependencyFileArg` is true. So I think it might be cleaner to keep that extra boolean so the code is easy to read and understand. What do you think?
> I think having the boolean variable is still more confusing (it adds more state to keep track of) than just handling the cases with if-statements and early continue.
> 
> How about:
> 
> ```
> // -M flags that take an arg..
> if (!UsingClDriver && (Arg == "-MF" || Arg == "-MT" || Arg == "-MQ")) {
>   ++i;
>   continue;
> }
> // -M flags blah blah
> if (!UsingClDriver && Arg.startswith("-M"))
>   continue;
> // MSVC flags blah blah
> if (Arg.startswith("/showIncludes") || Arg.startswith("-showIncludes"))
>   continue;
> 
> AdjustedArgs.push_back(Args[i]);
> ```
> 
> ?
Yup I agree that looks better :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86999