[PATCH] D29886: [clangd] Wire up ASTUnit and publish diagnostics with it.

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.


================
Comment at: clangd/ASTManager.cpp:69
+      // our one-element queue is empty.
+      if (!RequestIsPending && !Done)
+        ClangRequestCV.wait(Lock);
----------------
arphaman wrote:
> This is just a suggestion based on personal opinion: `RequestIsPending` and `Done` can be merged into one enum value that uses an enum like this:
> 
> ```
> enum ASTManagerState {
>   Waiting, ReceivedRequest, Done
> }
> ```
> 
> Then this condition could be rewritten as `if (state == Waiting)`.
I'm not so sure. Once we have a queue (iff we ever get a queue), the "request available" is an attribute of the queue, while the "we need to exit" is an attribute of the system.


================
Comment at: clangd/ASTManager.cpp:70
+      if (!RequestIsPending && !Done)
+        ClangRequestCV.wait(Lock);
+
----------------
arphaman wrote:
>  I believe `std::condition_variable` may also be unblocked spuriously when `wait` is called without a predicate, which would lead to an empty `File` down below.
Yep, +1, the general pattern is to put while loops around.
It might actually be simpler to write that with a queue from the start (note that this would make the request queueing side somewhat less simple):
  ... Lock(RequestLock);
  while (Requests.empty() && !Done) {
    ClangRequestCV.wait(Lock);
  }
  if (Done) return;
  File = Request.pop_front();
  ClangRequestCV.notify_all();
  ...

  the while loop  can be written somewhat simpler with the pred version:
  ClangRequestCV.wait(Lock, [&] { return !Request.empty() || Done; });



================
Comment at: clangd/ASTManager.cpp:116
+      Diagnostics.pop_back(); // Drop trailing comma.
+    this->Output.writeMessage(
+        R"({"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":")" +
----------------
One question is whether we want to couple this to JSON, or have structured output that we serialize.
(I'm happy with doing this now, and refining later - doesn't seem super hard to switch)


================
Comment at: clangd/ASTManager.h:34
+
+  void onDocumentAdd(StringRef Uri, const DocumentStore &Docs) override;
+  // FIXME: Implement onDocumentRemove
----------------
At a first glance, it's weird that we have a global document store, but get a document store for each notification. This at least needs documentation.


================
Comment at: clangd/ASTManager.h:67
+  /// Setting Done to true will make the worker thread terminate.
+  std::atomic<bool> Done;
+};
----------------
arphaman wrote:
> It looks like `Done` is always accessed in a scope where `RequestLock` is locked, so `atomic` doesn't seem needed here.
Yea, after only having read this header, it looks like we might want to pull out a Request as an abstraction.


================
Comment at: clangd/ClangDMain.cpp:32
+  auto *AST = new ASTManager(Out, Store);
+  Store.addListener(std::unique_ptr<ASTManager>(AST));
   JSONRPCDispatcher Dispatcher(llvm::make_unique<Handler>(Out));
----------------
krasimir wrote:
> Why not directly `Store.addListener(llvm::make_unique<ASTManager>(Out, Store));`?
Why's the ASTManager not on the stack?


================
Comment at: clangd/DocumentStore.h:39
+    for (const auto &Listener : Listeners)
+      Listener->onDocumentAdd(Uri, *this);
+  }
----------------
krasimir wrote:
> Is only the main thread supposed to addDocument-s? Otherwise the listener notifications might need to be guarded.
It would always be the responsibility of the callee to guard.


================
Comment at: clangd/DocumentStore.h:42
   /// Delete a document from the store.
-  void removeDocument(StringRef Uri) { Docs.erase(Uri); }
+  void removeDocument(StringRef Uri) { Docs.erase(Uri);
+    for (const auto &Listener : Listeners)
----------------
krasimir wrote:
> cierpuchaw wrote:
> > cierpuchaw wrote:
> > > Shouldn't this be called under a lock_guard?
> > I should've reworded my comment before submitting it. By 'this' I'm referring to the 'Docs.erase()' part, not the whole function.
> > 
> Even under a guard, it may potentially still be unsafe since workers may operate on erased/modified StringRef-s.
That depends on the guarantees made, and when we create copies :)


https://reviews.llvm.org/D29886