[Tooling] Improve handling of CL-style options

[hamzasood]

Summary

This patch fixes the handling of clang-cl options in InterpolatingCompilationDatabase.
They were previously ignored completely, which led to a lot of bugs:

• Additional options were being added with the wrong syntax. E.g. a file was specified as C++ by adding -x c++, which causes an error in CL mode.
• The args were parsed and then rendered, which means that the aliasing information was lost. E.g. /W3 was rendered to -Wall, which in CL mode means -Weverything.
• CL options were ignored when checking things like -std=, so a lot of logic was being bypassed.

[rnk]

lib/Tooling/InterpolatingCompilationDatabase.cpp

202–204

I think you need to do this up front, otherwise you'll parse some args in non-cl mode and some in cl mode.

[hamzasood]

Thanks, I've changed it so that the driver mode is calculated first.
I've also extended the unit test to include the various --driver-mode combinations.

[rnk]

lib/Tooling/InterpolatingCompilationDatabase.cpp

136

We support being called "CL.exe", but with our new VS integration, I don't think it matters to check for this case anymore. We may remove it over time.

140

Can you not keep using ParseArgs? It takes FlagsToInclude and FlagsToExclude, which you can compute outside the loop now.

156–169

I think the old for loop was nicer. I don't think this code needs to change, you should be able to use ParseArgs with the extra flag args.

[hamzasood]

lib/Tooling/InterpolatingCompilationDatabase.cpp

136

Should I add the check anyway?

140

Good point, I forgot to move the flags out of the loop when moving the —driver-mode check.

But I don’t think ParseArgs is suitable (see the other comment response).

156–169

The problem with the old loop is that we lose aliasing information (e.g. /W3 becomes -Wall). While ParseOneArg also performs alias conversion, it gives us indices for each individual argument. The last line of the new loop copies arguments by using that index information to read from OldArgs, which gives us the original spelling.

[sammccall]

Hi, sorry about overlooking cl mode flags when adding this, I was blissfully unaware that compile_commands.json could use that syntax :-)

Out of curiosity, are you using this with clangd or some other tool? I'm sure there are places where clangd injects unixy flags...
Will take a look and try to understand.

Be aware of D51314 which is fixing some nasty performance pitfalls of InterpolatingCompilationDatabase (it should be logic neutral, but moves around the parsing code).

Could you reupload the patch with context?

[hamzasood]

• Re-uploaded with full context.

Yeah, I encountered these issues while using clangd on Windows. I haven't run into any clangd issues after applying this patch, but admittedly my usage is very basic (pretty much just code completion). It may well be subtly broken in other places.

[sammccall]

Thanks for fixing this!
Mostly just picky style comments.
In particular, I know that some of the other maintainers here are just as ignorant of the cl driver as I am, and I want to make sure that it's still possible to follow the logic and debug unrelated problems without needing to know too much about it.

If some of the style bits is too annoying or unclear, happy to do some of it myself as a followup, let me know!

lib/Tooling/InterpolatingCompilationDatabase.cpp

124

nit: a two-value enum would be clearer and allow for terser variable names (Mode)

156

would you mind reverting this change and just wrapping the old Argv in an InputArgList?
I'm not sure the lifetime comments and std::transform aid readability here.

156–169

Makes sense, please add a comment summarizing. ("We don't use ParseArgs, as we must pass through the exact spelling of uninteresting args. Re-rendering is lossy for clang-cl options, e.g. /W3 -> -Wall")

175

these names are somewhat confusing - "End" neither marks the end of the loop nor the end of the current item (as it's initialized to Start).
What about:

for (unsigned Pos = 1; Pos < OldArgs.size();) {
  ...
  unsigned OldPos = Pos;
  Arg = ParseOneArg(..., Pos);
  ...
  NewArgs.insert(NewArgs.end(), &OldArgs[OldPos], &OldArgs[Pos]);
}

178

This seems a bit verbose.

First, do things actually break if we just use the default 0/0 masks? We're not trying to interpret all the flags, just a few and pass the rest through.

Failing that, it seems clearer to just use a ternary to initialize Included/Excluded, or inline them completely.
(Please also drop the extra scope here).

186

Arg.reset(OptTable->ParseOneArg...)

194

can we inline this and just || all the options together here?

197

please keep the mentions of -x etc even if not comprehensive - it's hard to precisely+tersely describe these flags in prose.

203

ditto --std

205

prefer *either* optional<> or allowing the function to return lang_unspecified, but not both. (There's no way a user can *explicitly* pass a flag saying the language is unspecified, right?)

225

as above, please revert comment

231

can we unify as addTypeFlag(TargetType, Mode, Result.CommandLine)?

258

Why do we need to take IsClMode into account while reading flags?
How would OPT__SLASH_Fa get parsed if we weren't in CL mode? Would we care?

(and below)

267

parseTypeArg?

283

parseStandardArg?

unittests/Tooling/CompilationDatabaseTest.cpp

652

I'm not sure the parameterized test is the right model here.
The INSTANTIATE_TEST_P and friends makes the test fixture pretty complicated, and failure messages hard to relate to input conditions.
Then the actual tests have a bunch of assertions conditional on which mode we're in.
Finally, we don't actually test any mixed-mode database.

Could we write this a little more directly?:

• pass the driver mode flag to add() in the normal way, in the Flags param
• require callers to "add" to pass "clang" or "clang-cl". (It's OK that this makes existing cases a bit more verbose)
• explicitly test the clang-cl and --driver-mode cases we care about, rather than running every assertion in every configuration

e.g.

TEST_F(InterpolateTest, ClangCL) {
  add("foo.cc", "clang");
  add("bar.cc", "clang-cl");
  add("baz.cc", "clang --driver-mode=clang-cl");

  EXPECT_EQ(getCommand("a/bar.h"), "clang-cl -D a/baz.cc /TP");
}

[hamzasood]

Thanks for the detailed feedback; I’ll try to get a new patch up in a few hours.

However I disagree with some of them (see replies).

lib/Tooling/InterpolatingCompilationDatabase.cpp

124

The advantage of a Boolean is that it makes the checks simpler. E.g. if (isCL) instead of if (mode == DriverMode::CL) or something.

Happy to change it though if you still disagree.

156

The change was more about safety than readability. The old approach left an InputArgList of dangling pointers after moving the new args into the cmd object. This way there’s no way to accidentally access deallocated memory by using the InputArgList.

As above, happy to change if you still disagree.

178

Theoretically it could break without the flags. We need to recognise input files and strip them from the command line. If someone on a UNIX platform has an input file called /W3, that’d instead be interpreted as a warning flag and it’ll be left in. Likewise for any file in the root directory with the same spelling as a CL-only argument.

But yeah, I’ll inline the flags with a ternary.

205

Kind of: -std=hello_there is parsed as an unspecified language. We still want to strip the flag in this case, which won’t be possible if we also use the unspecified value to denote a lack of an -std flag.

231

To clarify, you mean extract this into a helper function?

258

It was an optimisation attempt in that we can do 5 less comparisons in the case where we know that there aren’t going to be any CL flags. Maybe I was trying too hard to micro-optimise...

unittests/Tooling/CompilationDatabaseTest.cpp

652

The intent was to make sure that the existing cases (that shouldn’t depend on the driver mode) don’t break, which admittedly happened while I was working on the patch.

Most of the tests aren’t conditional on the mode, and I think it’s important that they’re tested in all configurations. The parameterisation also lets us test as clang-cl, clang-cl —driver-mode=cl, and clang —driver-mode=cl. Which are all valid ways of using CL mode.

[hamzasood]

I've addressed most of your comments and resolved the merge conflicts introduced in r340838.

[hamzasood]

lib/Tooling/InterpolatingCompilationDatabase.cpp

124

Also, there're more than just 2 driver modes. But we only care about cl and not cl.

156

I've restructured it so hopefully this is less of a concern.

unittests/Tooling/CompilationDatabaseTest.cpp

652

To clarify: the parameterisation runs the test 6 times. The isTestingCL() check narrows that down to 3.

[sammccall]

Thanks again for fixing this.
Still a few places I feel the code could be simpler, but will let you decide how to deal with them.
I would greatly appreciate removing the parameterized tests, as that's the place where I feel least confident I can understand exactly what the intended behavior is.

lib/Tooling/InterpolatingCompilationDatabase.cpp

124

I do find if (mode == DriverMode::CL) much clearer.

"CL" isn't a particularly clear name for people who don't deal with windows much, and "!isCL" isn't a great name for the GCC-compatible driver.

141

please remove premature optimizations (SmallVector, reserve) - this function is not (any more) performance-critical

143

simple loop is more readable than transform()

156

I think a comment // ArgList is no longer valid would suffice, or storing the ArgList in an Optional and resetting it.

In principle the restructuring seems fine, but it introduced new issues: the boundaries and data flow between the constructor and processInputCommandLine is unclear. (It reads from parameters which are derived from Cmd.CommandLine and overwrites Cmd.CommandLine itself, along with other state. Its name doesn't help clarify what its responsibility is.

If you want to keep this function separate, I'd suggest:

• make it static to avoid confusion about state while the constructor is running
• call it parseCommandLine to reflect the processing it's doing
• return tuple<vector<String>, DriverMode, LangStandard, Optional<Type>>
• call it as std::tie(Cmd.CommandLine, Mode, ...) = parseCommandLine(...)

But it also seems fine as part of the constructor.

192

can we just inline this ("--driver-mode") like we do with other specific we need in their string form (std etc)?

196

if (S.consume_front(...))
  return S == "cl";

231

Right - you already have the helper function toCLflag, I'd suggest extending/renaming it so it covers both CL and GCC and fits the call site more conveniently.

unittests/Tooling/CompilationDatabaseTest.cpp

652

Understood. I don't think the extra test coverage here is worth the infrastructure complexity, and would strongly prefer to add a handful of tests covering driver mode interactions instead.

[hamzasood]

Thanks for the help with this.
I reverted most of my changes (including the parameterised tests) before committing; I think I implemented all of your suggestions.

lib/Tooling/InterpolatingCompilationDatabase.cpp

 #include "clang/Frontend/LangStandard.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Option/ArgList.h"

   }
 }
 
+// Determine whether the given file path is the clang-cl executable.
+static bool checkIsCLMode(const std::vector<std::string> &CmdLine,
+                          const llvm::opt::OptTable &OptTable) {
+  const std::string DriverModeName =
+    OptTable.getOption(driver::options::OPT_driver_mode).getPrefixedName();
+
+  // First check for a --driver-mode override.
+  for (StringRef S : llvm::reverse(CmdLine)) {
+    if (S.startswith(DriverModeName))
+      return S.drop_front(DriverModeName.length()) == "cl";
+  }
+
+  // Otherwise just check the clang file name.
+  return llvm::sys::path::stem(CmdLine.front()).endswith_lower("clang-cl");
+}
+
 // A CompileCommand that can be applied to another file.
 struct TransferableCommand {
   // Flags that should not apply to all files are stripped from CommandLine.

   types::ID Type = types::TY_INVALID;
   // Standard specified by -std.
   LangStandard::Kind Std = LangStandard::lang_unspecified;
+  // Whether the command is for the CL driver.
+  bool IsCLMode = false;
 
   TransferableCommand(CompileCommand C)
       : Cmd(std::move(C)), Type(guessType(Cmd.Filename)) {
-    std::vector<std::string> NewArgs = {Cmd.CommandLine.front()};
+    // Stash the old arguments.
+    std::vector<std::string> OldArgs = std::move(Cmd.CommandLine);
+    Cmd.CommandLine.clear();
+
+    // Transform the command line to an llvm ArgList.
+    // Make sure that OldArgs lives for at least as long as this variable as the
+    // arg list contains pointers to the OldArgs strings.
+    llvm::opt::InputArgList ArgList;
+    {
+      // Unfortunately InputArgList requires an array of c-strings whereas we
+      // have an array of std::string; we'll need an intermediate vector.
+      SmallVector<const char *, 8> TmpArgv;
+      TmpArgv.reserve(OldArgs.size());
+      std::transform(OldArgs.begin(), OldArgs.end(),
+                     std::back_inserter(TmpArgv),
+                     [](const std::string &S) { return S.c_str(); });
+      ArgList = llvm::opt::InputArgList(TmpArgv.begin(), TmpArgv.end());
+    }
+
     // Parse the old args in order to strip out and record unwanted flags.
     auto OptTable = clang::driver::createDriverOptTable();
-    std::vector<const char *> Argv;
-    for (unsigned I = 1; I < Cmd.CommandLine.size(); ++I)
-      Argv.push_back(Cmd.CommandLine[I].c_str());
-    unsigned MissingI, MissingC;
-    auto ArgList = OptTable->ParseArgs(Argv, MissingI, MissingC);
-    for (const auto *Arg : ArgList) {
-      const auto &option = Arg->getOption();
+    IsCLMode = checkIsCLMode(OldArgs, *OptTable);
+    Cmd.CommandLine.emplace_back(OldArgs.front());
+    for (unsigned Start = 1, End = Start; End < OldArgs.size(); Start = End) {
+      std::unique_ptr<llvm::opt::Arg> Arg;
+      {
+        unsigned Included, Excluded;
+        if (IsCLMode) {
+          Included = driver::options::CoreOption | driver::options::CLOption;
+          Excluded = 0;
+        } else {
+          Included = 0;
+          Excluded = driver::options::CLOption;
+        }
+        Arg = std::unique_ptr<llvm::opt::Arg>(
+                  OptTable->ParseOneArg(ArgList, End, Included, Excluded));
+      }
+
+      if (!Arg)
+        continue;
+
       // Strip input and output files.
-      if (option.matches(clang::driver::options::OPT_INPUT) ||
-          option.matches(clang::driver::options::OPT_o)) {
+      if (isUntypedInputOrOutput(*Arg))
         continue;
-      }
-      // Strip -x, but record the overridden language.
-      if (option.matches(clang::driver::options::OPT_x)) {
-        for (const char *Value : Arg->getValues())
-          Type = types::lookupTypeForTypeSpecifier(Value);
+
+      // Strip type specifiers, but record the overridden language.
+      if (const auto GivenType = isTypeSpecArg(*Arg)) {
+        Type = *GivenType;
         continue;
       }
-      // Strip --std, but record the value.
-      if (option.matches(clang::driver::options::OPT_std_EQ)) {
-        for (const char *Value : Arg->getValues()) {
-          Std = llvm::StringSwitch<LangStandard::Kind>(Value)
-#define LANGSTANDARD(id, name, lang, desc, features)                           \
-  .Case(name, LangStandard::lang_##id)
-#define LANGSTANDARD_ALIAS(id, alias) .Case(alias, LangStandard::lang_##id)
-#include "clang/Frontend/LangStandards.def"
-                    .Default(Std);
-        }
+
+      // Strip std, but record the value.
+      if (const auto GivenStd = isStdArg(*Arg)) {
+        if (*GivenStd != LangStandard::lang_unspecified)
+          Std = *GivenStd;
         continue;
       }
-      llvm::opt::ArgStringList ArgStrs;
-      Arg->render(ArgList, ArgStrs);
-      NewArgs.insert(NewArgs.end(), ArgStrs.begin(), ArgStrs.end());
+
+      Cmd.CommandLine.insert(Cmd.CommandLine.end(),
+                             &OldArgs[Start], &OldArgs[End]);
     }
-    Cmd.CommandLine = std::move(NewArgs);
 
     if (Std != LangStandard::lang_unspecified) // -std take precedence over -x
       Type = toType(LangStandard::getLangStandardForKind(Std).getLanguage());

     Result.Filename = Filename;
     bool TypeCertain;
     auto TargetType = guessType(Filename, &TypeCertain);
-    // If the filename doesn't determine the language (.h), transfer with -x.
+    // If the filename doesn't determine the language (.h), use an appropriate
+    // argument to transfer it.
     if (!TypeCertain) {
       TargetType = types::onlyPrecompileType(TargetType) // header?
                        ? types::lookupHeaderTypeForSourceType(Type)
                        : Type;
-      Result.CommandLine.push_back("-x");
-      Result.CommandLine.push_back(types::getTypeName(TargetType));
+      if (IsCLMode) {
+        const StringRef Flag = toCLFlag(TargetType);
+        if (!Flag.empty())
+          Result.CommandLine.push_back(Flag);
+      } else {
+        Result.CommandLine.push_back("-x");
+        Result.CommandLine.push_back(types::getTypeName(TargetType));
+      }
     }
     // --std flag may only be transferred if the language is the same.
     // We may consider "translating" these, e.g. c++11 -> c11.
     if (Std != LangStandard::lang_unspecified && foldType(TargetType) == Type) {
       Result.CommandLine.push_back(
-          "-std=" +
+          (IsCLMode ? "/std:" : "-std=") +
           std::string(LangStandard::getLangStandardForKind(Std).getName()));
     }
     Result.CommandLine.push_back(Filename);

   }
 
 private:
+  bool isUntypedInputOrOutput(const llvm::opt::Arg &Arg) const {
+    const llvm::opt::Option &Opt = Arg.getOption();
+
+    if (Opt.matches(driver::options::OPT_INPUT))
+      return true;
+
+    if (IsCLMode)
+      return Opt.matches(driver::options::OPT__SLASH_Fa) ||
+             Opt.matches(driver::options::OPT__SLASH_Fe) ||
+             Opt.matches(driver::options::OPT__SLASH_Fi) ||
+             Opt.matches(driver::options::OPT__SLASH_Fo);
+    else
+      return Opt.matches(driver::options::OPT_o);
+  }
+
+  Optional<types::ID> isTypeSpecArg(const llvm::opt::Arg &Arg) const {
+    const llvm::opt::Option &Opt = Arg.getOption();
+    if (IsCLMode) {
+      if (Opt.matches(driver::options::OPT__SLASH_TC) ||
+          Opt.matches(driver::options::OPT__SLASH_Tc))
+        return types::TY_C;
+      if (Opt.matches(driver::options::OPT__SLASH_TP) ||
+          Opt.matches(driver::options::OPT__SLASH_Tp))
+        return types::TY_CXX;
+    } else {
+      if (Opt.matches(driver::options::OPT_x))
+        return types::lookupTypeForTypeSpecifier(Arg.getValue());
+    }
+    return None;
+  }
+
+  Optional<LangStandard::Kind> isStdArg(const llvm::opt::Arg &Arg) const {
+    const llvm::opt::Option &Opt = Arg.getOption();
+    if (Opt.matches(IsCLMode ? driver::options::OPT__SLASH_std
+                             : driver::options::OPT_std_EQ)) {
+      return llvm::StringSwitch<LangStandard::Kind>(Arg.getValue())
+#define LANGSTANDARD(id, name, lang, desc, features) \
+  .Case(name, LangStandard::lang_##id)
+#define LANGSTANDARD_ALIAS(id, alias) \
+  .Case(alias, LangStandard::lang_##id)
+#include "clang/Frontend/LangStandards.def"
+#undef LANGSTANDARD_ALIAS
+#undef LANGSTANDARD
+                 .Default(LangStandard::lang_unspecified);
+    }
+    return None;
+  }
+
   // Map the language from the --std flag to that of the -x flag.
   static types::ID toType(InputKind::Language Lang) {
     switch (Lang) {

       return types::TY_INVALID;
     }
   }
+
+  static StringRef toCLFlag(types::ID Type) {
+    types::isCXX(Type);
+    switch (Type) {
+    case types::TY_C:
+    case types::TY_CHeader:
+      return "/TC";
+    case types::TY_CXX:
+    case types::TY_CXXHeader:
+      return "/TP";
+    default:
+      return StringRef();
+    }
+  }
 };
 
 // CommandIndex does the real work: given a filename, it produces the best

unittests/Tooling/CompilationDatabaseTest.cpp

 #include "clang/Tooling/JSONCompilationDatabase.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Path.h"
+
+#define GTEST_HAS_COMBINE 1
 #include "gtest/gtest.h"
 
 namespace clang {

   }
 };
 
-class InterpolateTest : public ::testing::Test {
+enum class DriverExe { Clang, ClangCL };
+enum class DriverMode { None, GCC, CL };
+
+class InterpolateTest
+  : public ::testing::TestWithParam<std::tuple<DriverExe, DriverMode>> {
+
 protected:
+  bool isTestingCL() const {
+    switch (std::get<1>(GetParam())) {
+    case DriverMode::None: return std::get<0>(GetParam()) == DriverExe::ClangCL;
+    case DriverMode::GCC:  return false;
+    case DriverMode::CL:   return true;
+    default: llvm_unreachable("Invalid DriverMode");
+    }
+  }
+  StringRef getClangExe() const {
+    switch (std::get<0>(GetParam())) {
+    case DriverExe::Clang:   return "clang";
+    case DriverExe::ClangCL: return "clang-cl";
+    default: llvm_unreachable("Invalid DriverExe");
+    }
+  }
+  StringRef getDriverModeArg() const {
+    switch (std::get<1>(GetParam())) {
+    case DriverMode::None: return StringRef();
+    case DriverMode::GCC:  return "--driver-mode=gcc";
+    case DriverMode::CL:   return "--driver-mode=cl";
+    default: llvm_unreachable("Invalid DriverMode");
+    }
+  }
+
   // Adds an entry to the underlying compilation database.
   // A flag is injected: -D <File>, so the command used can be identified.
   void add(llvm::StringRef File, llvm::StringRef Flags = "") {
-    llvm::SmallVector<StringRef, 8> Argv = {"clang", File, "-D", File};
+    llvm::SmallVector<StringRef, 8> Argv;
+    Argv.push_back(getClangExe());
+    if (!getDriverModeArg().empty())
+      Argv.push_back(getDriverModeArg());
+    Argv.append({"-D", File});
     llvm::SplitString(Flags, Argv);
+
     llvm::SmallString<32> Dir;
     llvm::sys::path::system_temp_directory(false, Dir);
+
     Entries[path(File)].push_back(
         {Dir, path(File), {Argv.begin(), Argv.end()}, "foo.o"});
   }

             ->getCompileCommands(path(F));
     if (Results.empty())
       return "none";
-    // drop the input file argument, so tests don't have to deal with path().
-    EXPECT_EQ(Results[0].CommandLine.back(), path(F))
-        << "Last arg should be the file";
-    Results[0].CommandLine.pop_back();
-    return llvm::join(Results[0].CommandLine, " ");
+
+    ArrayRef<std::string> CmdLine = Results[0].CommandLine;
+
+    // Drop the clang executable path.
+    EXPECT_EQ(CmdLine.front(), getClangExe()) << "First arg should be clang";
+    CmdLine = CmdLine.drop_front();
+
+    // Drop the --driver-mode flag.
+    if (!getDriverModeArg().empty()) {
+      EXPECT_EQ(CmdLine.front(), getDriverModeArg())
+          << "Expected driver mode arg";
+      CmdLine = CmdLine.drop_front();
+    }
+
+    // Drop the input file.
+    EXPECT_EQ(CmdLine.back(), path(F)) << "Last arg should be the file";
+    CmdLine = CmdLine.drop_back();
+
+    return llvm::join(CmdLine, " ");
   }
 
   MemCDB::EntryMap Entries;
 };
 
-TEST_F(InterpolateTest, Nearby) {
+TEST_P(InterpolateTest, Nearby) {
   add("dir/foo.cpp");
   add("dir/bar.cpp");
   add("an/other/foo.cpp");
 
   // great: dir and name both match (prefix or full, case insensitive)
-  EXPECT_EQ(getCommand("dir/f.cpp"), "clang -D dir/foo.cpp");
-  EXPECT_EQ(getCommand("dir/FOO.cpp"), "clang -D dir/foo.cpp");
+  EXPECT_EQ(getCommand("dir/f.cpp"), "-D dir/foo.cpp");
+  EXPECT_EQ(getCommand("dir/FOO.cpp"), "-D dir/foo.cpp");
   // no name match. prefer matching dir, break ties by alpha
-  EXPECT_EQ(getCommand("dir/a.cpp"), "clang -D dir/bar.cpp");
+  EXPECT_EQ(getCommand("dir/a.cpp"), "-D dir/bar.cpp");
   // an exact name match beats one segment of directory match
   EXPECT_EQ(getCommand("some/other/bar.h"),
-            "clang -D dir/bar.cpp -x c++-header");
+            isTestingCL()
+                ? "-D dir/bar.cpp /TP"
+                : "-D dir/bar.cpp -x c++-header");
   // two segments of directory match beat a prefix name match
-  EXPECT_EQ(getCommand("an/other/b.cpp"), "clang -D an/other/foo.cpp");
+  EXPECT_EQ(getCommand("an/other/b.cpp"), "-D an/other/foo.cpp");
   // if nothing matches at all, we still get the closest alpha match
   EXPECT_EQ(getCommand("below/some/obscure/path.cpp"),
-            "clang -D an/other/foo.cpp");
+            "-D an/other/foo.cpp");
 }
 
-TEST_F(InterpolateTest, Language) {
-  add("dir/foo.cpp", "-std=c++17");
-  add("dir/baz.cee", "-x c");
+TEST_P(InterpolateTest, Language) {
+  add("dir/foo.cpp", isTestingCL() ? "/std:c++17" : "-std=c++17");
+  add("dir/baz.cee", isTestingCL() ? "/TC" : "-x c");
 
   // .h is ambiguous, so we add explicit language flags
   EXPECT_EQ(getCommand("foo.h"),
-            "clang -D dir/foo.cpp -x c++-header -std=c++17");
+            isTestingCL()
+                ? "-D dir/foo.cpp /TP /std:c++17"
+                : "-D dir/foo.cpp -x c++-header -std=c++17");
   // and don't add -x if the inferred language is correct.
-  EXPECT_EQ(getCommand("foo.hpp"), "clang -D dir/foo.cpp -std=c++17");
+  EXPECT_EQ(getCommand("foo.hpp"),
+            isTestingCL()
+                ? "-D dir/foo.cpp /std:c++17"
+                : "-D dir/foo.cpp -std=c++17");
   // respect -x if it's already there.
-  EXPECT_EQ(getCommand("baz.h"), "clang -D dir/baz.cee -x c-header");
+  EXPECT_EQ(getCommand("baz.h"),
+            isTestingCL()
+                ? "-D dir/baz.cee /TC"
+                : "-D dir/baz.cee -x c-header");
   // prefer a worse match with the right language
-  EXPECT_EQ(getCommand("foo.c"), "clang -D dir/baz.cee");
+  EXPECT_EQ(getCommand("foo.c"), "-D dir/baz.cee");
   Entries.erase(path(StringRef("dir/baz.cee")));
   // Now we transfer across languages, so drop -std too.
-  EXPECT_EQ(getCommand("foo.c"), "clang -D dir/foo.cpp");
+  EXPECT_EQ(getCommand("foo.c"), "-D dir/foo.cpp");
 }
 
-TEST_F(InterpolateTest, Strip) {
-  add("dir/foo.cpp", "-o foo.o -Wall");
-  // the -o option and the input file are removed, but -Wall is preserved.
-  EXPECT_EQ(getCommand("dir/bar.cpp"), "clang -D dir/foo.cpp -Wall");
+TEST_P(InterpolateTest, Strip) {
+  // The input and output are removed, but the warning level is preserved.
+  if (isTestingCL()) {
+    add("dir/foo.cpp", "/Fofoo.o /W3");
+    EXPECT_EQ(getCommand("dir/bar.cpp"), "-D dir/foo.cpp /W3");
+  } else {
+    add("dir/foo.cpp", "-o foo.o -Wall");
+    EXPECT_EQ(getCommand("dir/bar.cpp"), "-D dir/foo.cpp -Wall");
+  }
 }
 
-TEST_F(InterpolateTest, Case) {
+TEST_P(InterpolateTest, Case) {
   add("FOO/BAR/BAZ/SHOUT.cc");
   add("foo/bar/baz/quiet.cc");
   // Case mismatches are completely ignored, so we choose the name match.
-  EXPECT_EQ(getCommand("foo/bar/baz/shout.C"), "clang -D FOO/BAR/BAZ/SHOUT.cc");
+  EXPECT_EQ(getCommand("foo/bar/baz/shout.C"), "-D FOO/BAR/BAZ/SHOUT.cc");
 }
 
+INSTANTIATE_TEST_CASE_P(RegularAndCL, InterpolateTest,
+  ::testing::Combine(
+    ::testing::Values(DriverExe::Clang, DriverExe::ClangCL),
+    ::testing::Values(DriverMode::None, DriverMode::GCC, DriverMode::CL)));
+
 TEST(CompileCommandTest, EqualityOperator) {
   CompileCommand CCRef("/foo/bar", "hello.c", {"a", "b"}, "hello.o");
   CompileCommand CCTest = CCRef;