bkramer added inline comments.
================
Comment at: clangd/ASTManager.cpp:21
+using namespace clang;
+using namespace clangd;
+
----------------
ioeric wrote:
> Any reason not to wrap code in namespaces instead?
I don't really have an opinion one way or the other, but this seems to be the
preferred way of doing this in LLVM.
================
Comment at: clangd/ASTManager.cpp:70
+ if (!RequestIsPending && !Done)
+ ClangRequestCV.wait(Lock);
+
----------------
klimek wrote:
> 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; });
>
Added the lambda. The current state is pretty weird, but making it a queue just
to drop random elements from that queue is even weirder :(
================
Comment at: clangd/ASTManager.cpp:148
+ std::string Error;
+ I = tooling::CompilationDatabase::autoDetectFromSource(Uri, Error);
+ return I.get();
----------------
krasimir wrote:
> Do you have an idea about a proper error handling if Error?
For now I'll send it to logs()
================
Comment at: clangd/ASTManager.h:34
+
+ void onDocumentAdd(StringRef Uri, const DocumentStore &Docs) override;
+ // FIXME: Implement onDocumentRemove
----------------
klimek wrote:
> 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.
This is leftover from an earlier design. Removed.
================
Comment at: clangd/ASTManager.h:67
+ /// Setting Done to true will make the worker thread terminate.
+ std::atomic<bool> Done;
+};
----------------
klimek wrote:
> 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.
Wrapped all the guarded variables in a struct.
================
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));
----------------
klimek wrote:
> krasimir wrote:
> > Why not directly `Store.addListener(llvm::make_unique<ASTManager>(Out,
> > Store));`?
> Why's the ASTManager not on the stack?
Fixed. Lifetime is no longer managed by the DocumentStore (that was a bit weird)
================
Comment at: clangd/DocumentStore.h:39
+ for (const auto &Listener : Listeners)
+ Listener->onDocumentAdd(Uri, *this);
+ }
----------------
klimek wrote:
> 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.
Added guard for now.
================
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)
----------------
klimek wrote:
> 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 :)
Added guard.
================
Comment at: clangd/DocumentStore.h:51
auto I = Docs.find(Uri);
return I == Docs.end() ? StringRef("") : StringRef(I->second);
}
----------------
krasimir wrote:
> StringRef doesn't own the underlying data, right? What if I erase it in the
> meantime?
I guess it makes sense to create copies here. Somewhat sad but safe.
================
Comment at: clangd/DocumentStore.h:63
+ AllDocs.emplace_back(P.first(), P.second);
+ return AllDocs;
+ }
----------------
krasimir wrote:
> Same thing, shouldn't these be strings?
Done.
https://reviews.llvm.org/D29886
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
