https://github.com/mpark created https://github.com/llvm/llvm-project/pull/115438
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. >From 78b08dae9386435f734861a2f5428c88ca9be1e4 Mon Sep 17 00:00:00 2001 From: Michael Park <mcyp...@gmail.com> Date: Thu, 7 Nov 2024 17:12:23 -0800 Subject: [PATCH] Fix the modification detection logic in `ClangdLSPServer::applyConfiguration`. `OverlayCDB::getCompileCommand` applies a mangling operation if the CDB has one set. Given a `CommandMangler`, the provided command line is mangled to have additional flags such as `--driver-mode=g++`. Example: https://github.com/llvm/llvm-project/blob/d4525b016f5a1ab2852acb2108742b2f9d0bd3bd/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp#L46-L62 Considering the previous logic of: ``` auto Old = CDB->getCompileCommand(File); auto New = tooling::CompileCommand(...); if (Old != New) { ... } ``` The `Old != New` here is always true because `New` is the "raw" compile command whereas `Old` is the "cooked" (mangling aplied) compile command. This diff performs the change check inside of `setCompileCommand` instead to compare the "raw" compile commands. --- clang-tools-extra/clangd/ClangdLSPServer.cpp | 13 +++++------ .../clangd/GlobalCompilationDatabase.cpp | 15 +++++++++---- .../clangd/GlobalCompilationDatabase.h | 4 +++- .../GlobalCompilationDatabaseTests.cpp | 22 ++++++++++--------- 4 files changed, 31 insertions(+), 23 deletions(-) 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()); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
