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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
