[PATCH] D44408: Move DraftMgr from ClangdServer to ClangdLSPServer

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Mar 15 07:38:42 PDT 2018

ilya-biryukov added inline comments.

Comment at: clangd/ClangdLSPServer.cpp:495
+llvm::Optional<std::string> ClangdLSPServer::getDocument(PathRef File) {
+  llvm::Optional<std::string> Contents = DraftMgr.getDraft(File);
+  if (!Contents)
This function is equivalent to `return DraftMgr.getDraft()`. Remove and replace all usages with `DraftMgr.getDraft()`?

Comment at: clangd/ClangdServer.cpp:154
                                  llvm::Expected<InputsAndPreamble> IP) {
     assert(IP && "error when trying to read preamble for codeComplete");
     auto PreambleData = IP->Preamble;
Please replace this assert with:
if (!IP)
  return CB(IP.takeError());

(Otherwise the assert will fire when completion is called for non-tracked files, we really want this to be an error instead)

Comment at: clangd/ClangdServer.h:121
   /// compilation database even if they were cached in previous invocations.
-  void addDocument(PathRef File, StringRef Contents,
+  void addDocument(PathRef File, std::string Contents,
                    WantDiagnostics WD = WantDiagnostics::Auto,
Let's keep it a `StringRef`. It means an extra copy for now, `DraftMgr` return `StringRef`s instead if we remove mutexes from its interface.

Comment at: clangd/ClangdServer.h:225
+  /// Document contents with a version of this content.
+  struct VersionedContents {
+    DocVersion Version;
We don't use `VersionedContents` anywhere. Maybe remove this struct?

Comment at: clangd/ClangdServer.h:236
   FileSystemProvider &FSProvider;
-  DraftStore DraftMgr;
+  llvm::StringMap<DocVersion> InternalVersion;
   // The index used to look up symbols. This could be:
This is probably a good place for a small comment:
/// Used to synchronize diagnostic responses for added and removed files.

Comment at: clangd/DraftStore.h:36
+  void
+  forEachActiveDraft(std::function<void(PathRef, StringRef)> Callback) const;
Let's avoid calling `Callback` while holding a lock, it is very easy to come up with a code that deadlocks.

I suggest either keeping the `getActiveFiles()` function or  removing the mutex and the locks. IIRC, the `DraftStore` is never  actually used concurrently, so removing mutexes should be no-op change.

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list