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

[Hans Wennborg via Phabricator via cfe-commits]

hans 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"))
----------------
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]);
```

?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86999