[clangd] Fix the modification detection logic in ClangdLSPServer::applyConfiguration.
#115438
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…uration`. `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.
|
@llvm/pr-subscribers-clangd @llvm/pr-subscribers-clang-tools-extra Author: Michael Park (mpark) ChangesThe current llvm-project/clang-tools-extra/clangd/ClangdLSPServer.cpp Lines 1424 to 1432 in 1adca7a
This means that the This PR proposes to fix this by moving the check into Full diff: https://github.com/llvm/llvm-project/pull/115438.diff 4 Files Affected:
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());
}
|
dmpolukhin
approved these changes
Nov 11, 2024
There was a problem hiding this comment.
I think it is unexpected side effect of https://reviews.llvm.org/D143436. The diff was written together https://reviews.llvm.org/D148663 that was rejected so this edge was accidentally broken.
mpark
changed the title
ClangdLSPServer::applyConfiguration.ClangdLSPServer::applyConfiguration.
HighCommander4
approved these changes
Nov 15, 2024
There was a problem hiding this comment.
Thanks, this LGTM.
It would be nice to have a test exercising applyConfiguration itself, but I realize we don't currently have such tests (and that the intended effect -- that an open file whose command did not change does not get re-parsed -- is not trivial for check for).
It someone would like to contribute such test coverage in a future patch, that would certainly be welcome.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current
ClangdLSPServer::applyConfigurationlogic tests whether the stored CDB for a file has changed, like this:llvm-project/clang-tools-extra/clangd/ClangdLSPServer.cpp
Lines 1424 to 1432 in 1adca7a
OverlayCDB::getCompileCommandapplies a mangling operation if the CDB has one set. ACommandManglermangles the provided command line, for example, adding flags such as--driver-mode=g++. See this example test case:llvm-project/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
Lines 53 to 61 in d4525b0
This means that the
Old != Newtest is always true becauseNewis a "raw" (pre-mangling) compile command whereasOlda "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.