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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.
Herald added a reviewer: jkorous-apple.

Overall looks good! Some ideas about code structure inlined.



================
Comment at: clangd/CodeComplete.cpp:327
+// The CompletionRecorder captures Sema code-complete output, including context.
+// It filters out ignored results (and therefore does fuzzy-matching of names).
+// It doesn't do scoring or conversion to CompletionItem yet, as we want to
----------------
Does the recorder still do fuzzy-matching?


================
Comment at: clangd/CodeComplete.cpp:339
+  std::vector<CodeCompletionResult> Results;
+  CodeCompletionContext CCContext;
+  Sema *Sema = nullptr; // The Sema that created the results.
----------------
It seems that `CCContext` can change  during `ProcessCodeCompleteResults`. It's unclear what the implication is for `codeCompletionString` at the end. 


================
Comment at: clangd/CodeComplete.cpp:434
+  // Returns true if we had more than N candidates, and dropped some.
+  bool more() const { return More; }
 
----------------
Maybe `dropped()`? 


================
Comment at: clangd/CodeComplete.cpp:679
+//   - TopN determines the results with the best score.
 CompletionList codeComplete(const Context &Ctx, PathRef FileName,
                             const tooling::CompileCommand &Command,
----------------
The overall behavior looks good. And the comments really help understand the code! As chatted offline, we might want to break down this function, and a class that book-keeps all the states might be helpful. 


================
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
----------------
I think we already only query namespace scopes now?


================
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()};
----------------
As discussed offline, we want to defer unqualified completion support (IIRC?) until we have enough information about visible scopes (i.e. after D42073).


================
Comment at: clangd/CodeComplete.cpp:782
+      // For Sema results, we build the CCS here, where we have the arenas.
+      auto *CCS =
+          Candidate.first.SemaResult
----------------
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`.


================
Comment at: clangd/CodeComplete.cpp:789
+      auto &R = Output.items.back();
+      R.scoreInfo = Candidate.second;
+      R.sortText = sortText(R.scoreInfo->finalScore, R.label);
----------------
Would it make sense to move the score construction into `build`, by passing in necessary information in `Candidate.second`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42181