hans added a comment.
Seems reasonable to me. Just some nits.
================
Comment at: clang/lib/Tooling/ArgumentsAdjusters.cpp:29
+ auto EqualSignIndex = ArgRef.find('=');
+ return StringRef(ArgRef.data() + EqualSignIndex + 1);
+ }
----------------
Instead of manually indexing into the StringRef, the I think the if-statement
could be replaced by
```
if (ArgRef.consume_front("--driver-mode="))
return ArgRef;
```
================
Comment at: clang/lib/Tooling/ArgumentsAdjusters.cpp:32
+ }
+ return StringRef();
+}
----------------
I'd drop the Optional and just let the empty StringRef signal not finding the
driver mode.
================
Comment at: clang/lib/Tooling/ArgumentsAdjusters.cpp:108
+ auto DriverMode = getDriverMode(Args);
+ auto UsingClDriver = (DriverMode.hasValue() && *DriverMode == "cl");
+
----------------
If getDriverMode() didn't return Optional, this could be simplified to
```
bool UsingClDriver = (getDriverMode() == "cl");
```
================
Comment at: clang/lib/Tooling/ArgumentsAdjusters.cpp:115
// -MF, -MG, -MP, -MT, -MQ, -MD, and -MMD.
- if (!Arg.startswith("-M") && !Arg.startswith("/showIncludes") &&
- !Arg.startswith("-showIncludes")) {
+ // Dependency-file options in MSVC do not begin with -M, so we
+ // do not remove those when using the cl driver.
----------------
This line and the "All dependency-file options begin with -M" line above seem
to contradict each other. Perhaps the could be united somehow.
================
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"))
----------------
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]);
```
================
Comment at: clang/lib/Tooling/ArgumentsAdjusters.cpp:132
if (Arg == "-MF" || Arg == "-MT" || Arg == "-MQ")
// These flags take an argument: -MX foo. Skip the next argument also.
----------------
Need to handle -MT too I suppose
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
