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