[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/ClangdServer.cpp:159
+  }
+  if (SpecFuzzyReq) {
+    if (auto Filter = speculateCompletionFilter(Content, Pos)) {
----------------
ilya-biryukov wrote:
> NIT: maybe invert condition to reduce nesting?
It would become something like:
```
if (!SpecFuzzyReq)
  return SpecFuzzyReq;
.... // some work
return SpecFuzzyReq;
```

Having to return the same thing twice seems a bit worse IMO.


================
Comment at: clangd/CodeComplete.cpp:1388
+  if (Len > 0)
+    St++; // Shift to the first valid character.
+  return Content.substr(St, Len);
----------------
ilya-biryukov wrote:
> This looks incorrect in case of the identifiers starting at offset 0. A corner case, but still.
`Offset == 0` is handled above.


================
Comment at: clangd/tool/ClangdMain.cpp:179
+        should be effective for a number of code completions.)"),
+    llvm::cl::init(true));
+
----------------
ilya-biryukov wrote:
> Maybe remove this option?
> Doing a speculative request seems like the right thing to do in all cases. It never hurts completion latency. Any reasons to avoid it?
Sure, was trying to be conservative :)

Removed the command-line option but still keeping the code completion option. As async call has some overhead, I think we should only enable it when static index is provided.


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:926
 
-  const std::vector<FuzzyFindRequest> allRequests() const { return Requests; }
+  const std::vector<FuzzyFindRequest> consumeRequests() const {
+    auto Reqs = std::move(Requests);
----------------
ilya-biryukov wrote:
> Why do we want to change this? To avoid tests getting requests from the previous tests?
Yes. 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962