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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
