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

[Benjamin Kramer via Phabricator via cfe-commits]

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