ilya-biryukov added inline comments.
================
Comment at: clangd/ClangdServer.h:232
+ /// addDocument. Used to avoid races when sending diagnostics to the clients.
+ static Key<ClangdServer::DocVersion> DocVersionKey;
----------------
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?
================
Comment at: clangd/TUScheduler.h:92
+ ASTRetentionPolicy RetentionPolicy,
+ DiagsCallback OnDiags = nullptr);
~TUScheduler();
----------------
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.
================
Comment at: unittests/clangd/TUSchedulerTests.cpp:32
-void ignoreUpdate(Optional<std::vector<Diag>>) {}
void ignoreError(Error Err) {
handleAllErrors(std::move(Err), [](const ErrorInfoBase &) {});
----------------
sammccall wrote:
> (up to you if you feel like fixing while here)
> this is just llvm::consumeError
Will fix in a separate commit, like it more when cleanups are the only thing in
the commit.
================
Comment at: unittests/clangd/TUSchedulerTests.cpp:38
+/// cancelled.
+void updateWithCallback(TUScheduler &S, PathRef File, ParseInputs Inputs,
+ WantDiagnostics WD, llvm::unique_function<void()> CB) {
----------------
sammccall wrote:
> ParseInputs is always getInputs(File), you could do that here (similarly for
> `updateWithDiags`) to avoid repetition.
It's actually a bit more complicated: `getInputs(File, Contents)`, note the
second parameter.
Done anyway, definitely looks better (and less prone to mistakes!).
The calls are now `updateWith(S, File, Contents...)`, there's one place that
still relies on the `Inputs` though (checks the VFS does not change in the
following read).
================
Comment at: unittests/clangd/TUSchedulerTests.cpp:495
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
- ASTRetentionPolicy());
+ ASTRetentionPolicy(), captureDiags);
----------------
sammccall wrote:
> can we just increment a counter in the callback, and have DoUpdate look for a
> delta?
Maybe... I haven't looked closely into the test, would prefer to keep this
change mostly mechanical and avoid digging into the test logic.
Happy to take a look into simplifying the test separately, of course.
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
