[PATCH] D116721: [Tooling] When transferring compile commands between files, always use '--'

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: usaxena95, arphaman.
sammccall requested review of this revision.
Herald added projects: clang, clang-tools-extra.
Herald added a subscriber: cfe-commits.

"driver <flags> -- <input>" is a particularly convenient form of the
compile command to manipulate, with fewer special cases to handle.

Guaranteeing that the output command is of that form is cheap and makes
it easier to consume the result in some cases.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116721

Files:
  clang-tools-extra/clangd/CompileCommands.cpp
  clang/include/clang/Tooling/CompilationDatabase.h
  clang/lib/Tooling/InterpolatingCompilationDatabase.cpp
  clang/unittests/Tooling/CompilationDatabaseTest.cpp


Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===================================================================
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -739,6 +739,9 @@
     EXPECT_EQ(Results[0].CommandLine.back(), MakeNative ? path(F) : F)
         << "Last arg should be the file";
     Results[0].CommandLine.pop_back();
+    EXPECT_EQ(Results[0].CommandLine.back(), "--")
+        << "Second-last arg should be --";
+    Results[0].CommandLine.pop_back();
     return llvm::join(Results[0].CommandLine, " ");
   }
 
@@ -826,18 +829,6 @@
   EXPECT_EQ(getCommand("dir/bar.cpp"), "clang -D dir/foo.cpp -Wall -std=c++14");
 }
 
-TEST_F(InterpolateTest, InsertDoubleDash) {
-  add("dir/foo.cpp", "-o foo.o -std=c++14 -Wall");
-  EXPECT_EQ(getCommand("-dir/bar.cpp", false),
-            "clang -D dir/foo.cpp -Wall -std=c++14 --");
-}
-
-TEST_F(InterpolateTest, InsertDoubleDashForClangCL) {
-  add("dir/foo.cpp", "clang-cl", "/std:c++14 /W4");
-  EXPECT_EQ(getCommand("/dir/bar.cpp", false),
-            "clang-cl -D dir/foo.cpp /W4 /std:c++14 --");
-}
-
 TEST_F(InterpolateTest, Case) {
   add("FOO/BAR/BAZ/SHOUT.cc");
   add("foo/bar/baz/quiet.cc");
@@ -879,7 +870,7 @@
   CompileCommand Transferred = transferCompileCommand(std::move(Cmd), "foo.h");
   EXPECT_EQ(Transferred.Filename, "foo.h");
   EXPECT_THAT(Transferred.CommandLine,
-              ElementsAre("clang", "-Wall", "-x", "c++-header", "foo.h"));
+              ElementsAre("clang", "-Wall", "-x", "c++-header", "--", "foo.h"));
   EXPECT_EQ(Transferred.Directory, "dir");
 }
 
Index: clang/lib/Tooling/InterpolatingCompilationDatabase.cpp
===================================================================
--- clang/lib/Tooling/InterpolatingCompilationDatabase.cpp
+++ clang/lib/Tooling/InterpolatingCompilationDatabase.cpp
@@ -243,8 +243,7 @@
           llvm::Twine(ClangCLMode ? "/std:" : "-std=") +
           LangStandard::getLangStandardForKind(Std).getName()).str());
     }
-    if (Filename.startswith("-") || (ClangCLMode && Filename.startswith("/")))
-      Result.CommandLine.push_back("--");
+    Result.CommandLine.push_back("--");
     Result.CommandLine.push_back(std::string(Filename));
     return Result;
   }
Index: clang/include/clang/Tooling/CompilationDatabase.h
===================================================================
--- clang/include/clang/Tooling/CompilationDatabase.h
+++ clang/include/clang/Tooling/CompilationDatabase.h
@@ -216,6 +216,8 @@
 /// Transforms a compile command so that it applies the same configuration to
 /// a different file. Most args are left intact, but tweaks may be needed
 /// to certain flags (-x, -std etc).
+///
+/// The output command will always end in {"--", Filename}.
 tooling::CompileCommand transferCompileCommand(tooling::CompileCommand,
                                                StringRef Filename);
 
Index: clang-tools-extra/clangd/CompileCommands.cpp
===================================================================
--- clang-tools-extra/clangd/CompileCommands.cpp
+++ clang-tools-extra/clangd/CompileCommands.cpp
@@ -290,16 +290,9 @@
     TransferCmd.CommandLine = std::move(Cmd);
     TransferCmd = transferCompileCommand(std::move(TransferCmd), File);
     Cmd = std::move(TransferCmd.CommandLine);
-
-    // Restore the canonical "driver --opts -- filename" form we expect.
-    // FIXME: This is ugly and coupled. Make transferCompileCommand ensure it?
-    assert(!Cmd.empty() && Cmd.back() == File);
-    Cmd.pop_back();
-    if (!Cmd.empty() && Cmd.back() == "--")
-      Cmd.pop_back();
-    assert(!llvm::is_contained(Cmd, "--"));
-    Cmd.push_back("--");
-    Cmd.push_back(File.str());
+    assert(Cmd.size() >= 2 && Cmd.back() == File &&
+           Cmd[Cmd.size() - 2] == "--" &&
+           "TransferCommand should produce a command ending in -- filename");
   }
 
   for (auto &Edit : Config::current().CompileFlags.Edits)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D116721.397779.patch
Type: text/x-patch
Size: 3987 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220106/58399957/attachment.bin>