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

[Hans Wennborg via Phabricator via llvm-commits]

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