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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
