[clangd] Fix the modification detection logic in ClangdLSPServer::applyConfiguration. (#115438)

Prior to this, the "old != new" check would always evaluate to true because it was comparing a pre-mangling new command to a post-mangling old command.

Author: mpark

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);
     }
   }

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

@@ -807,20 +807,27 @@ 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
   // doesn't receive different names for the same 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)

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);
 

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());
 }