llvmbot wrote:

<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-tools-extra

Author: Michael Park (mpark)

<details>
<summary>Changes</summary>

The current `ClangdLSPServer::applyConfiguration` logic tests whether the 
stored CDB for a file has changed.

https://github.com/llvm/llvm-project/blob/1adca7af21f1d8cc12b0f1c33db8ab869b36ae48/clang-tools-extra/clangd/ClangdLSPServer.cpp#L1424-L1432

`OverlayCDB::getCompileCommand` applies a mangling operation if the CDB has one 
set. A `CommandMangler` mangles the provided command line, for example, adding 
flags such as `--driver-mode=g++`. See this example test case:

https://github.com/llvm/llvm-project/blob/d4525b016f5a1ab2852acb2108742b2f9d0bd3bd/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp#L53-L61

This means that the `Old != New` test is **always** true because `New` is a 
"raw" (pre-mangling) compile command whereas `Old` a "cooked" (post-mangling) 
compile command.

This PR proposes to fix this by moving the check into `setCompileCommand`. The 
comparison is done between the "raw" compile commands, and a boolean 
representing whether there has been a change in the command line is returned.

---
Full diff: https://github.com/llvm/llvm-project/pull/115438.diff


4 Files Affected:

- (modified) clang-tools-extra/clangd/ClangdLSPServer.cpp (+5-8) 
- (modified) clang-tools-extra/clangd/GlobalCompilationDatabase.cpp (+11-4) 
- (modified) clang-tools-extra/clangd/GlobalCompilationDatabase.h (+3-1) 
- (modified) 
clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp (+12-10) 


``````````diff
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp 
b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index 06573a57554245..05dd313d0a0d35 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -1419,15 +1419,12 @@ void ClangdLSPServer::applyConfiguration(
     const ConfigurationSettings &Settings) {
   // Per-file update to the compilation database.
   llvm::StringSet<> ModifiedFiles;
-  for (auto &Entry : Settings.compilationDatabaseChanges) {
-    PathRef File = Entry.first;
-    auto Old = CDB->getCompileCommand(File);
-    auto New =
-        tooling::CompileCommand(std::move(Entry.second.workingDirectory), File,
-                                std::move(Entry.second.compilationCommand),
+  for (auto &[File, Command] : Settings.compilationDatabaseChanges) {
+    auto Cmd =
+        tooling::CompileCommand(std::move(Command.workingDirectory), File,
+                                std::move(Command.compilationCommand),
                                 /*Output=*/"");
-    if (Old != New) {
-      CDB->setCompileCommand(File, std::move(New));
+    if (CDB->setCompileCommand(File, std::move(Cmd))) {
       ModifiedFiles.insert(File);
     }
   }
diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp 
b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
index 1d96667a8e9f4a..71e97ac4efd673 100644
--- a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -807,7 +807,7 @@ tooling::CompileCommand 
OverlayCDB::getFallbackCommand(PathRef File) const {
   return Cmd;
 }
 
-void OverlayCDB::setCompileCommand(PathRef File,
+bool OverlayCDB::setCompileCommand(PathRef File,
                                    std::optional<tooling::CompileCommand> Cmd) 
{
   // We store a canonical version internally to prevent mismatches between set
   // and get compile commands. Also it assures clients listening to broadcasts
@@ -815,12 +815,19 @@ void OverlayCDB::setCompileCommand(PathRef File,
   std::string CanonPath = removeDots(File);
   {
     std::unique_lock<std::mutex> Lock(Mutex);
-    if (Cmd)
-      Commands[CanonPath] = std::move(*Cmd);
-    else
+    if (Cmd) {
+      if (auto [It, Inserted] =
+              Commands.try_emplace(CanonPath, std::move(*Cmd));
+          !Inserted) {
+        if (It->second == *Cmd)
+          return false;
+        It->second = *Cmd;
+      }
+    } else
       Commands.erase(CanonPath);
   }
   OnCommandChanged.broadcast({CanonPath});
+  return true;
 }
 
 DelegatingCDB::DelegatingCDB(const GlobalCompilationDatabase *Base)
diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.h 
b/clang-tools-extra/clangd/GlobalCompilationDatabase.h
index ea999fe8aee017..f8349c6efecb01 100644
--- a/clang-tools-extra/clangd/GlobalCompilationDatabase.h
+++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.h
@@ -203,7 +203,9 @@ class OverlayCDB : public DelegatingCDB {
   tooling::CompileCommand getFallbackCommand(PathRef File) const override;
 
   /// Sets or clears the compilation command for a particular file.
-  void
+  /// Returns true if the command was changed (including insertion and 
removal),
+  /// false if it was unchanged.
+  bool
   setCompileCommand(PathRef File,
                     std::optional<tooling::CompileCommand> CompilationCommand);
 
diff --git 
a/clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp 
b/clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
index a2ffdefe1bbcb2..c9e01e52dac1f3 100644
--- a/clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
+++ b/clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
@@ -92,11 +92,13 @@ TEST_F(OverlayCDBTest, GetCompileCommand) {
   EXPECT_EQ(CDB.getCompileCommand(testPath("missing.cc")), std::nullopt);
 
   auto Override = cmd(testPath("foo.cc"), "-DA=3");
-  CDB.setCompileCommand(testPath("foo.cc"), Override);
+  EXPECT_TRUE(CDB.setCompileCommand(testPath("foo.cc"), Override));
+  EXPECT_FALSE(CDB.setCompileCommand(testPath("foo.cc"), Override));
   EXPECT_THAT(CDB.getCompileCommand(testPath("foo.cc"))->CommandLine,
               Contains("-DA=3"));
   EXPECT_EQ(CDB.getCompileCommand(testPath("missing.cc")), std::nullopt);
-  CDB.setCompileCommand(testPath("missing.cc"), Override);
+  EXPECT_TRUE(CDB.setCompileCommand(testPath("missing.cc"), Override));
+  EXPECT_FALSE(CDB.setCompileCommand(testPath("missing.cc"), Override));
   EXPECT_THAT(CDB.getCompileCommand(testPath("missing.cc"))->CommandLine,
               Contains("-DA=3"));
 }
@@ -111,7 +113,7 @@ TEST_F(OverlayCDBTest, NoBase) {
   OverlayCDB CDB(nullptr, {"-DA=6"});
   EXPECT_EQ(CDB.getCompileCommand(testPath("bar.cc")), std::nullopt);
   auto Override = cmd(testPath("bar.cc"), "-DA=5");
-  CDB.setCompileCommand(testPath("bar.cc"), Override);
+  EXPECT_TRUE(CDB.setCompileCommand(testPath("bar.cc"), Override));
   EXPECT_THAT(CDB.getCompileCommand(testPath("bar.cc"))->CommandLine,
               Contains("-DA=5"));
 
@@ -128,10 +130,10 @@ TEST_F(OverlayCDBTest, Watch) {
     Changes.push_back(ChangedFiles);
   });
 
-  Inner.setCompileCommand("A.cpp", tooling::CompileCommand());
-  Outer.setCompileCommand("B.cpp", tooling::CompileCommand());
-  Inner.setCompileCommand("A.cpp", std::nullopt);
-  Outer.setCompileCommand("C.cpp", std::nullopt);
+  EXPECT_TRUE(Inner.setCompileCommand("A.cpp", tooling::CompileCommand()));
+  EXPECT_TRUE(Outer.setCompileCommand("B.cpp", tooling::CompileCommand()));
+  EXPECT_TRUE(Inner.setCompileCommand("A.cpp", std::nullopt));
+  EXPECT_TRUE(Outer.setCompileCommand("C.cpp", std::nullopt));
   EXPECT_THAT(Changes, ElementsAre(ElementsAre("A.cpp"), ElementsAre("B.cpp"),
                                    ElementsAre("A.cpp"), 
ElementsAre("C.cpp")));
 }
@@ -151,7 +153,7 @@ TEST_F(OverlayCDBTest, Adjustments) {
   tooling::CompileCommand BarCommand;
   BarCommand.Filename = testPath("bar.cc");
   BarCommand.CommandLine = {"clang++", "-DB=1", testPath("bar.cc")};
-  CDB.setCompileCommand(testPath("bar.cc"), BarCommand);
+  EXPECT_TRUE(CDB.setCompileCommand(testPath("bar.cc"), BarCommand));
   Cmd = *CDB.getCompileCommand(testPath("bar.cc"));
   EXPECT_THAT(
       Cmd.CommandLine,
@@ -412,7 +414,7 @@ TEST(GlobalCompilationDatabaseTest, NonCanonicalFilenames) {
 
   llvm::SmallString<128> Root(testRoot());
   llvm::sys::path::append(Root, "build", "..", "a.cc");
-  DB.setCompileCommand(Root.str(), tooling::CompileCommand());
+  EXPECT_TRUE(DB.setCompileCommand(Root.str(), tooling::CompileCommand()));
   EXPECT_THAT(DiscoveredFiles, UnorderedElementsAre(testPath("a.cc")));
   DiscoveredFiles.clear();
 
@@ -432,7 +434,7 @@ TEST_F(OverlayCDBTest, GetProjectInfo) {
   EXPECT_EQ(DB.getProjectInfo(Header)->SourceRoot, testRoot());
 
   // Shouldn't change after an override.
-  DB.setCompileCommand(File, tooling::CompileCommand());
+  EXPECT_TRUE(DB.setCompileCommand(File, tooling::CompileCommand()));
   EXPECT_EQ(DB.getProjectInfo(File)->SourceRoot, testRoot());
   EXPECT_EQ(DB.getProjectInfo(Header)->SourceRoot, testRoot());
 }

``````````

</details>


https://github.com/llvm/llvm-project/pull/115438
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to