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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: clangd/ASTManager.cpp:69
+      // our one-element queue is empty.
+      if (!RequestIsPending && !Done)
+        ClangRequestCV.wait(Lock);
----------------
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)`.


================
Comment at: clangd/ASTManager.cpp:70
+      if (!RequestIsPending && !Done)
+        ClangRequestCV.wait(Lock);
+
----------------
 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.


================
Comment at: clangd/ASTManager.h:67
+  /// Setting Done to true will make the worker thread terminate.
+  std::atomic<bool> Done;
+};
----------------
It looks like `Done` is always accessed in a scope where `RequestLock` is locked, so `atomic` doesn't seem needed here.


https://reviews.llvm.org/D29886