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