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

Eric Liu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jan 18 05:37:05 PST 2018

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`?

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list