[PATCH] D44408: Move DraftMgr from ClangdServer to ClangdLSPServer

[Ilya Biryukov via Phabricator via cfe-commits]

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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44408