[PATCH] D53642: [clangd] Don't invalidate LSP-set compile commands when closing a file.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE345231: [clangd] Don't invalidate LSP-set compile 
commands when closing a file. (authored by sammccall, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53642?vs=170861&id=171033#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53642

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h


Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -135,10 +135,5 @@
   return false;
 }
 
-void InMemoryCompilationDb::invalidate(PathRef File) {
-  std::unique_lock Lock(Mutex);
-  Commands.erase(File);
-}
-
 } // namespace clangd
 } // namespace clang
Index: clangd/ClangdLSPServer.h
===
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -113,8 +113,6 @@
 static CompilationDB
 makeDirectoryBased(llvm::Optional CompileCommandsDir);
 
-void invalidate(PathRef File);
-
 /// Sets the compilation command for a particular file.
 /// Only valid for in-memory CDB, no-op and error log on DirectoryBasedCDB.
 ///
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -100,10 +100,6 @@
   bool setCompilationCommandForFile(PathRef File,
 tooling::CompileCommand 
CompilationCommand);
 
-  /// Removes the compilation command for \p File if it's present in the
-  /// mapping.
-  void invalidate(PathRef File);
-
 private:
   mutable std::mutex Mutex;
   llvm::StringMap Commands; /* GUARDED_BY(Mut) */
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -393,7 +393,6 @@
 // fail rather than giving wrong results.
 DraftMgr.removeDraft(File);
 Server->removeDocument(File);
-CDB->invalidate(File);
 elog("Failed to update {0}: {1}", File, Contents.takeError());
 return;
   }
@@ -489,7 +488,6 @@
   PathRef File = Params.textDocument.uri.file();
   DraftMgr.removeDraft(File);
   Server->removeDocument(File);
-  CDB->invalidate(File);
 }
 
 void ClangdLSPServer::onDocumentOnTypeFormatting(
@@ -804,11 +802,6 @@
/*IsDirectoryBased=*/true);
 }
 
-void ClangdLSPServer::CompilationDB::invalidate(PathRef File) {
-  if (!IsDirectoryBased)
-static_cast(CDB.get())->invalidate(File);
-}
-
 bool ClangdLSPServer::CompilationDB::setCompilationCommandForFile(
 PathRef File, tooling::CompileCommand CompilationCommand) {
   if (IsDirectoryBased) {


Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -135,10 +135,5 @@
   return false;
 }
 
-void InMemoryCompilationDb::invalidate(PathRef File) {
-  std::unique_lock Lock(Mutex);
-  Commands.erase(File);
-}
-
 } // namespace clangd
 } // namespace clang
Index: clangd/ClangdLSPServer.h
===
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -113,8 +113,6 @@
 static CompilationDB
 makeDirectoryBased(llvm::Optional CompileCommandsDir);
 
-void invalidate(PathRef File);
-
 /// Sets the compilation command for a particular file.
 /// Only valid for in-memory CDB, no-op and error log on DirectoryBasedCDB.
 ///
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -100,10 +100,6 @@
   bool setCompilationCommandForFile(PathRef File,
 tooling::CompileCommand CompilationCommand);
 
-  /// Removes the compilation command for \p File if it's present in the
-  /// mapping.
-  void invalidate(PathRef File);
-
 private:
   mutable std::mutex Mutex;
   llvm::StringMap Commands; /* GUARDED_BY(Mut) */
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -393,7 +393,6 @@
 // fail rather than giving wrong results.
 DraftMgr.removeDraft(File);
 Server->removeDocument(File);
-CDB->invalidate(File);
 elog("Failed to update {0}: {1}", File, Contents.takeError());
 return;
   }
@@ -489,7 +488,6 @@
   PathRef File = Params.textDocument.uri.file();
   DraftMgr.removeDraft(File);
   Server->removeDocument(File);
-  CDB->invalidate(File);
 }
 
 void ClangdLSPServer::onDocumentOnTypeFormatting(
@@ -804,11 +

[PATCH] D53642: [clangd] Don't invalidate LSP-set compile commands when closing a file.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM. Good catch!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53642



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53642: [clangd] Don't invalidate LSP-set compile commands when closing a file.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ioeric.

It doesn't make much sense: setting them is not coupled to opening the file,
it's an asynchronous notification.

I don't think this is a breaking change - this behavior is hard to observe!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53642

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h


Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -100,10 +100,6 @@
   bool setCompilationCommandForFile(PathRef File,
 tooling::CompileCommand 
CompilationCommand);
 
-  /// Removes the compilation command for \p File if it's present in the
-  /// mapping.
-  void invalidate(PathRef File);
-
 private:
   mutable std::mutex Mutex;
   llvm::StringMap Commands; /* GUARDED_BY(Mut) */
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -135,10 +135,5 @@
   return false;
 }
 
-void InMemoryCompilationDb::invalidate(PathRef File) {
-  std::unique_lock Lock(Mutex);
-  Commands.erase(File);
-}
-
 } // namespace clangd
 } // namespace clang
Index: clangd/ClangdLSPServer.h
===
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -113,8 +113,6 @@
 static CompilationDB
 makeDirectoryBased(llvm::Optional CompileCommandsDir);
 
-void invalidate(PathRef File);
-
 /// Sets the compilation command for a particular file.
 /// Only valid for in-memory CDB, no-op and error log on DirectoryBasedCDB.
 ///
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -354,7 +354,6 @@
 // fail rather than giving wrong results.
 DraftMgr.removeDraft(File);
 Server->removeDocument(File);
-CDB->invalidate(File);
 elog("Failed to update {0}: {1}", File, Contents.takeError());
 return;
   }
@@ -450,7 +449,6 @@
   PathRef File = Params.textDocument.uri.file();
   DraftMgr.removeDraft(File);
   Server->removeDocument(File);
-  CDB->invalidate(File);
 }
 
 void ClangdLSPServer::onDocumentOnTypeFormatting(
@@ -765,11 +763,6 @@
/*IsDirectoryBased=*/true);
 }
 
-void ClangdLSPServer::CompilationDB::invalidate(PathRef File) {
-  if (!IsDirectoryBased)
-static_cast(CDB.get())->invalidate(File);
-}
-
 bool ClangdLSPServer::CompilationDB::setCompilationCommandForFile(
 PathRef File, tooling::CompileCommand CompilationCommand) {
   if (IsDirectoryBased) {


Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -100,10 +100,6 @@
   bool setCompilationCommandForFile(PathRef File,
 tooling::CompileCommand CompilationCommand);
 
-  /// Removes the compilation command for \p File if it's present in the
-  /// mapping.
-  void invalidate(PathRef File);
-
 private:
   mutable std::mutex Mutex;
   llvm::StringMap Commands; /* GUARDED_BY(Mut) */
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -135,10 +135,5 @@
   return false;
 }
 
-void InMemoryCompilationDb::invalidate(PathRef File) {
-  std::unique_lock Lock(Mutex);
-  Commands.erase(File);
-}
-
 } // namespace clangd
 } // namespace clang
Index: clangd/ClangdLSPServer.h
===
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -113,8 +113,6 @@
 static CompilationDB
 makeDirectoryBased(llvm::Optional CompileCommandsDir);
 
-void invalidate(PathRef File);
-
 /// Sets the compilation command for a particular file.
 /// Only valid for in-memory CDB, no-op and error log on DirectoryBasedCDB.
 ///
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -354,7 +354,6 @@
 // fail rather than giving wrong results.
 DraftMgr.removeDraft(File);
 Server->removeDocument(File);
-CDB->invalidate(File);
 elog("Failed to update {0}: {1}", File, Contents.takeError());
 return;
   }
@@ -450,7 +449,6 @@
   PathRef File = Params.textDocument.uri.file();
   DraftMgr.removeDraft(File);
   Server->removeDocument(File);
-  CDB->invalidate(File);
 }
 
 void Clan