[clang-tools-extra] Fix the modification detection logic in `ClangdLSPServer::applyConfiguration`. (PR #115438)

2024-11-11 Thread Dmitry Polukhin via cfe-commits 

https://github.com/dmpolukhin approved this pull request.

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.

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

[clang-tools-extra] Fix the modification detection logic in `ClangdLSPServer::applyConfiguration`. (PR #115438)

2024-11-07 Thread via cfe-commits 

llvmbot wrote:




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

Author: Michael Park (mpark)


Changes

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 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 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 CompilationCommand);
 
diff --git 
a/clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp 
b/clang-tools-extra/clangd/unittests/GlobalC

[clang-tools-extra] Fix the modification detection logic in `ClangdLSPServer::applyConfiguration`. (PR #115438)

2024-11-07 Thread Michael Park via cfe-commits 

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 
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 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 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(