[PATCH] D42181: [clangd] Merge index-provided completions with those from Sema.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Oops, I forgot to submit comments with my last update.
I don't think there's anything surprising so I'll land this, but let me know if you want any changes!



================
Comment at: clangd/CodeComplete.cpp:339
+  std::vector<CodeCompletionResult> Results;
+  CodeCompletionContext CCContext;
+  Sema *Sema = nullptr; // The Sema that created the results.
----------------
ioeric wrote:
> It seems that `CCContext` can change  during `ProcessCodeCompleteResults`. It's unclear what the implication is for `codeCompletionString` at the end. 
This function is only ever called once, but this isn't documented anywhere :-\
Added an assert.


================
Comment at: clangd/CodeComplete.cpp:734
+    // Now we have the needed context to query the index.
+    // FIXME: we shouldn't query the index if the scope is e.g. class members.
+    // FIXME: in addition to querying for extra/overlapping symbols, we should
----------------
ioeric wrote:
> I think we already only query namespace scopes now?
Nope, we never check the completion context kind, and I think I've seen cases where we end up completing when it's inappropriate.

Fixed this (see the new `allowIndex()` check) and removed the fixme.


================
Comment at: clangd/CodeComplete.cpp:747
+      // FIXME(ioeric): add scopes based on using directives and enclosing ns.
+      if (auto SS = Recorder->CCContext.getCXXScopeSpecifier())
+        Req.Scopes = {getSpecifiedScope(*Recorder->Sema, **SS).forIndex()};
----------------
ioeric wrote:
> As discussed offline, we want to defer unqualified completion support (IIRC?) until we have enough information about visible scopes (i.e. after D42073).
Done - we now complete only symbols in the global scope.
I added two FIXMEs describing how we can progressively make this better over time. Do they match your understanding?


================
Comment at: clangd/CodeComplete.cpp:782
+      // For Sema results, we build the CCS here, where we have the arenas.
+      auto *CCS =
+          Candidate.first.SemaResult
----------------
ioeric wrote:
> It would be a bit more natural if the decision of building CCS is hidden in the candidate. Maybe pass in a callback for creating CCS from a sema result and let candidate decide whether to create CCS or not? This would also get rid of the constraint for `build`.
I agree, but trying this out I found it equally hard to read, and not as decoupled as I'd hope.
So as discussed offline, sticking with the ugly-and-direct approach :-)


================
Comment at: clangd/CodeComplete.cpp:743
+  int NSema = 0, NIndex = 0, NBoth = 0; // Counters for logging.
+  bool Incomplete = false;
+  llvm::Optional<FuzzyMatcher> Filter; // Initialized once Sema runs.
----------------
ioeric wrote:
> `InComplete` can probably be output variable of `queryIndex` and `addCandidate` instead of a state?
Certainly it can (it needs to be an out-param, because these functions already have primary return values). Just as these could all be free functions :-)

I tried it out - I find the out-params are a bit messy/hard to read, and they'd need to be added to `mergeResults`, `queryIndex` and `addCandidate`. It adds quite a lot of noise, and I'm not sure on balance emphasizing the flow of IsIncomplete is worth obscuring the flow of the results themselves. If you disagree, let me know (or just change it!)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42181