sammccall added a comment.
Nice!
The biggest suggestion I have is merging the callback into ASTCallbacks. That's
awkward in initialization (`makeUpdateCallbacks` probably needs to be called
unconditionally with a maybe-null pointer) but it seems like a natural grouping
in all the places it gets plumbed around to... and there are a lot!
================
Comment at: clangd/ClangdServer.h:232
+ /// addDocument. Used to avoid races when sending diagnostics to the clients.
+ static Key<ClangdServer::DocVersion> DocVersionKey;
----------------
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
================
Comment at: clangd/TUScheduler.h:92
+ ASTRetentionPolicy RetentionPolicy,
+ DiagsCallback OnDiags = nullptr);
~TUScheduler();
----------------
ISTM the callback would fit nicely on the ASTCallbacks?
It even gets plumbed through to ASTWorker correctly by reference.
================
Comment at: unittests/clangd/TUSchedulerTests.cpp:32
-void ignoreUpdate(Optional<std::vector<Diag>>) {}
void ignoreError(Error Err) {
handleAllErrors(std::move(Err), [](const ErrorInfoBase &) {});
----------------
(up to you if you feel like fixing while here)
this is just llvm::consumeError
================
Comment at: unittests/clangd/TUSchedulerTests.cpp:36
+/// Updates a file and executes a callback when the update is finished or
+/// cancelled.
----------------
Yeah, these are messy. I also don't see something better in general.
There's a place or two below where I think we can avoid them without much
verbosity, which I think is probably worthwhile even if lots of tests need them
:-/
================
Comment at: unittests/clangd/TUSchedulerTests.cpp:38
+/// cancelled.
+void updateWithCallback(TUScheduler &S, PathRef File, ParseInputs Inputs,
+ WantDiagnostics WD, llvm::unique_function<void()> CB) {
----------------
ParseInputs is always getInputs(File), you could do that here (similarly for
`updateWithDiags`) to avoid repetition.
================
Comment at: unittests/clangd/TUSchedulerTests.cpp:495
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
- ASTRetentionPolicy());
+ ASTRetentionPolicy(), captureDiags);
----------------
can we just increment a counter in the callback, and have DoUpdate look for a
delta?
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
