[clang-tools-extra] Fix the modification detection logic in `ClangdLSPServer::applyConfiguration`. (PR #115438)
Michael Park via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 7 23:51:17 PST 2024
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 <mcypark at 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());
}
More information about the cfe-commits
mailing list