sammccall added a comment.
Looks pretty good. I think we can build the main file & preamble in parallel.
Otherwise nits.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:946
+ // gurantee eventual consistency.
+ if (LatestPreamble && Config::current().Diagnostics.AllowStalePreamble)
+ generateDiagnostics(std::make_unique<CompilerInvocation>(*Invocation),
----------------
We should probably kick off the preamble rebuild before doing this synchronous
AST rebuild, so the two can run in parallel.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1131
+ // Use latest file contents instead of the preamble inputs whenever we can
+ // emit stale diagnostics. This ensures diagnostics are newer backwards,
+ // e.g. after emitting stale diagnostics for version N + 1, if a preamble
----------------
This explanation focuses on the potential problems (why the other behavior is
wrong) and on the AllowStalePreamble mode only. I think it would be more
enlightening to talk both about problems and correctness, and about the old
mode and the new mode.
Maybe:
```
The file may have been edited since we started building this preamble.
If diagnostics need a fresh preamble, we must use the old version that matches
the preamble. We make forward progress as updatePreamble() receives increasing
versions, and this is the only place we emit diagnostics.
If diagnostics can use a stale preamble, we use the current contents of the
file instead.
This provides more up-to-date diagnostics, and avoids diagnostics going
backwards (we may have already emitted staler-preamble diagnostics for the new
version). We still have eventual consistency: at some point updatePreamble()
will catch up to the current file.
```
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1202
+TEST_F(TUSchedulerTests, PublishWithStalePreamble) {
+ class BlockPreambleThread : public ParsingCallbacks {
+ public:
----------------
class comment?
// Callbacks that blocks the preamble thread after the first preamble is built.
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1225
+
+ std::optional<std::pair<std::string, std::string>>
+ WaitForNewDiags(TUScheduler &S, PathRef File, ParseInputs PI) {
----------------
having a method on the PreambleCallbacks which calls update() on the scheduler
seems pretty confusing - outside its responsibilities.
I'd suggest just moving this method and its associated state out of the class,
and have onMainAST just invoke a callback. (I see the appeal of wrapping this
in a class, I just don't think it should be *this* class)
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1236
+ });
+ if (!ReceivedDiags)
+ return std::nullopt;
----------------
this nullopt stuff isn't used in the test, ADD_FAILURE and return something
random instead?
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1260
+
+ Notification BlockPreamble;
+ auto DiagCallbacks = std::make_unique<BlockPreambleThread>(BlockPreamble);
----------------
this should rather be *Unblock*Preamble, I think, as that's what triggering it
does?
I'd suggest using this name rather than `N` inside the class too, would make it
a bit less mysterious
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1272
+ PI.Version = PI.Contents = "1";
+ ASSERT_THAT(BlockForDiags(PI), testing::Pair("1", PI.Version));
+
----------------
nit: Pair("1", "1") - tests should be explicit rather than DRY
maybe even `Pair(/*Preamble=*/"1", /*Main=*/"1")`
(and in the next two - the "eventual consistency" assert is probably fine as-is
I think)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144456/new/
https://reviews.llvm.org/D144456
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
