sammccall added a comment.
API looks good. Implementation looks like it can be simplified a bit, unless
I'm missing something.
================
Comment at: clangd/CodeComplete.h:117
+ /// (e.g. preamble is still being built).
+ bool AllowFallbackMode = false;
};
----------------
nit: "mode" doesn't add anything here
================
Comment at: clangd/TUScheduler.cpp:50
#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Tooling/CompilationDatabase.h"
----------------
not needed or bad patch base?
================
Comment at: clangd/TUScheduler.cpp:51
+#include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/Optional.h"
----------------
not needed?
================
Comment at: clangd/TUScheduler.cpp:881
+ // asynchronous mode, as TU update should finish before this is run.
+ if (!It->second->Worker->isFirstPreambleBuilt() &&
+ Consistency == StaleOrAbsent && PreambleTasks) {
----------------
Does this need to be a special case at the top of TUScheduler::runWithPreamble,
rather than unified with the rest of the body?
e.g. the `if (!PreambleTasks)` appears to block appears to handle that case
correctly already, and the `Consistency == Consistent` block won't trigger.
I think all you need to do is make the `Worker->waitForFirstPreamble()` line
conditional?
(This means no changes to ASTWorker, Notification etc)
================
Comment at: clangd/TUScheduler.h:190
/// Schedule an async read of the preamble.
- /// If there's no preamble yet (because the file was just opened), we'll wait
- /// for it to build. The result may be null if it fails to build or is empty.
- /// If an error occurs, it is forwarded to the \p Action callback.
- /// Context cancellation is ignored and should be handled by the Action.
- /// (In practice, the Action is almost always executed immediately).
+ /// If there's no preamble yet (because the file was just opened), we'll
+ /// either run \p Action without preamble (if \p Consistency is
----------------
This sentence has grown unwieldy:can we just say "If there's no up-to-date
preamble, we follow the PreambleConsistency policy"?
(I personally find the \p s hurt readability, but up to you)
================
Comment at: clangd/TUScheduler.h:193
+ /// `StaleOrAbsent`) or wait for it to build. The result may be null, if it
+ /// fails to build, or empty if there's no include. If an error occurs, it is
+ /// forwarded to the \p Action callback. Context cancellation is ignored and
----------------
bad reflow, break before "if an error occurs" (or just drop the sentence)
================
Comment at: clangd/TUScheduler.h:194
+ /// fails to build, or empty if there's no include. If an error occurs, it is
+ /// forwarded to the \p Action callback. Context cancellation is ignored and
+ /// should be handled by the Action. (In practice, the Action is almost
always
----------------
I think this is a bad reflow, need a line break before "Context cancellation is
ignored..."
================
Comment at: unittests/clangd/ClangdTests.cpp:1068
+
+ // XXX provide a broken command first and then a good command.
+ auto FooCpp = testPath("foo.cpp");
----------------
does this comment need an update?
================
Comment at: unittests/clangd/ClangdTests.cpp:1082
+ EXPECT_EQ(Res.Context, CodeCompletionContext::CCC_Recovery);
+ ASSERT_TRUE(Server.blockUntilIdleForTest());
+ EXPECT_THAT(cantFail(runCodeComplete(Server, FooCpp, Code.point(),
----------------
do I understand right that this test is currently racy?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59811/new/
https://reviews.llvm.org/D59811
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
