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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.cpp:159
+  }
+  if (SpecFuzzyReq) {
+    if (auto Filter = speculateCompletionFilter(Content, Pos)) {
----------------
ioeric wrote:
> 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.
I think it's better. Besides, you could replace return `llvm::None` in the first statement (would be even more straightforward from my point of view).
There's a secion in [[ https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code | LLVM Style Guide ]] on this.


================
Comment at: clangd/CodeComplete.cpp:1388
+  if (Len > 0)
+    St++; // Shift to the first valid character.
+  return Content.substr(St, Len);
----------------
ioeric wrote:
> 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.
Didn't notice. LG, thanks


================
Comment at: clangd/CodeComplete.h:186
+
+struct SpeculativeFuzzyFind {
+  /// A cached request from past code completions.
----------------
Maybe a short comment describing on why we have this parameter?


================
Comment at: clangd/CodeComplete.h:196
+  /// Set by `codeComplete()`.
+  llvm::Optional<FuzzyFindRequest> NewReq;
+  /// Result from the speculative/asynchonous call. This is only valid if
----------------
`NewReq` is an implementation detail only needed in `CodeComplete.cpp`, right? Maybe remove from this struct and only set in .cpp file?


================
Comment at: clangd/CodeComplete.h:197
+  llvm::Optional<FuzzyFindRequest> NewReq;
+  /// Result from the speculative/asynchonous call. This is only valid if
+  /// `SpecReq` is set.
----------------
Maybe remove description of the value in the result?
We don't want anyone to use, so a simple comment that the future is only there to wait until the async calls complete should be enough.


================
Comment at: clangd/CodeComplete.h:217
+             std::shared_ptr<PCHContainerOperations> PCHs,
+             SpeculativeFuzzyFind *SpecFuzzyFind, CodeCompleteOptions Opts);
 
----------------
SpecFuzzyFind is partially an out parameter, maybe put it at the end of the parameter list?


================
Comment at: clangd/tool/ClangdMain.cpp:179
+        should be effective for a number of code completions.)"),
+    llvm::cl::init(true));
+
----------------
ioeric wrote:
> 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.
Enabling only when static index is there LG. OTOH, it should always take less than code completion in case there's no static index, so it would've probably been fine either way


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:928
+    auto Reqs = std::move(Requests);
+    Requests.clear();
+    return Reqs;
----------------
Calling any methods on moved-from objects is a red flag.
It probably works on vectors, but maybe reassign `Requests = {}` or use a swap-with-empty-vector trick instead to avoid that?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962