[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

[Sam McCall via Phabricator via cfe-commits]

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