[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM! See the NITs, specifically about `runAddDocument` - those definitely look worth fixing.



================
Comment at: unittests/clangd/ClangdTests.cpp:1148
+  EXPECT_EQ(Res.Context, CodeCompletionContext::CCC_Recovery);
+  // Identifier-based fallback completion doesn't know about "symbol" scope.
+  EXPECT_THAT(Res.Completions,
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > Not strictly related to this patch, but maybe we could add a flag to completion results to indicate if the completion happened via a fallback mode or not?
> > 
> > Would make the test code more straightforward and the tests like these would not rely on a particular implementation of the fallback mode (e.g. I can imagine the fallback mode learning about the scopes later on)
> We are setting the context to `Recovery` and make fallback as part of `Recovery`. Do you we should distinguish fallback mode from `Recovery`?
Ah, `Recovery` looks good enough if we check the same location twice and second should be non-recovery.

Maybe keep **only** the `Context == Recovery` check? Checking for particular results only seems to make test less focused. 


================
Comment at: unittests/clangd/ClangdTests.cpp:1113
+    getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override {
+      CanReturnCommand.wait();
+      auto FileName = llvm::sys::path::filename(File);
----------------
Ah, we should really have a wait-with-timeout for these use-cases.
It's sad that we'll block indefinitely in the old implementation at this point. Having a failing test is much better than the one that never finishes.

No need to do anything in this patch, though, that requires a change to `Threading.h` that is best done separately anyway.


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:1392
+  // code completion.
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
   CodeCompleteResult Completions = cantFail(runCodeComplete(
----------------
Same here: could you use `runAddDocument`?


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:1390
   Server.addDocument(FooCpp, Source.code(), WantDiagnostics::Yes);
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
   CodeCompleteResult Completions = cantFail(runCodeComplete(
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > Could you expand why we need this?
> Added a comment.
Ah, thanks! Could we instead use a sync helper here to keep the code a bit simpler (won't need a comment too)?
```
runAddDocument(Server, ...);
```


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60607/new/

https://reviews.llvm.org/D60607