[PATCH] D54760: [clangd] Cleanup: make the diags callback global in TUScheduler

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.cpp:77
+    FileIndex *FIndex,
+    llvm::unique_function<void(PathRef, std::vector<Diag>)> OnDiags) {
+  using DiagsCallback = decltype(OnDiags);
----------------
sammccall wrote:
> hmm, the double-indirection for diags seems a bit funny, especially since we have a dedicated method on ClangdServer so the lambda is trivial.
> 
> Maybe we should just make the CB class nested or friend in ClangdServer, and pass it a ClangdServer*? Then it can access DynamicIdx and consumeDiagnostics directly. The decoupling here isn't that strong to start with.
I would avoid doing that, because ClangdServer is not thread-safe and giving the async callbacks that run on a different threads an access to the full class is scary.
Giving a narrower interface seems better, even if that means double-indirection

After D54829, there's no need for double-indirection too, the callback is simply directly calling into the diagnostics consumer, so it shouldn't be an issue.


================
Comment at: clangd/ClangdServer.h:232
+  /// addDocument. Used to avoid races when sending diagnostics to the clients.
+  static Key<ClangdServer::DocVersion> DocVersionKey;
 
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > I'm not sure using context here buys much: there aren't many layers, they're fairly coupled, and this information would look pretty natural in the interfaces.
> > > 
> > > What about:
> > >  - move the definition of DocVersion to TUScheduler
> > >  - make DocVersion a member of ParseInputs
> > >  - pass (PathRef, DocVersion, vector<Diag>) to the TUScheduler's diag callback
> > I'd keep it separate: `ParseInputs` is defined in `ClangdUnit.cpp` and it serves the purpose of defining everything we need to run the compiler.
> > Adding `DocVersion` there would make no sense: it's of no use to the actual code doing preamble builds or parsing.
> > 
> > I would rather aim to get rid of `DocVersion` (for this particular purpose, there are other ways to fix the raciness that the DocVersions workaround).
> > WDYT?
> Yeah, separating it from ParseInputs makes sense conceptually, and getting rid of it is indeed better.
> 
> I don't think this justifies using Context.
> 
> Passing it as a separate parameter seems fine to me. Putting it in ParseInputs with a FIXME is a little less principled but avoids interface churn.
The versions are not too hard to remove, here's the change that does this: D54829
That would avoid the need for putting them into either the context or the `ParseInputs`.


================
Comment at: clangd/TUScheduler.cpp:156
 class ASTWorker {
+private:
   friend class ASTWorkerHandle;
----------------
sammccall wrote:
> nit: we generally have the private section at the top without explicitly introducing it (as common in LLVM), or at the bottom. I'm fine with either style but would prefer not to add a third.
Yeah, sorry, that was an accidental change.


================
Comment at: clangd/TUScheduler.h:92
+              ASTRetentionPolicy RetentionPolicy,
+              DiagsCallback OnDiags = nullptr);
   ~TUScheduler();
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > ISTM the callback would fit nicely on the ASTCallbacks?
> > > It even gets plumbed through to ASTWorker correctly by reference.
> > Done. This is definitely less plumbing, but IMO the public interface looks a bit worse now.
> > Consuming ASTs and diagnostics are two independent concerns and it's a bit awkward to combine them in the same struct.
> > 
> > But we don't have too many callers, updating those is easy.
> (Separate yes, but I'm not sure they're that much more separate than consuming preambles vs ASTs, really. Outside of the 2/1 split of where the data goes today...)
> but I'm not sure they're that much more separate than consuming preambles vs ASTs
ASTs and resulting diagnostics are on different layers of abstraction, so I don't think they fit well in the same interface, even if the data-flow for those is similar.



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54760