[PATCH] D48762: [clangd] codeComplete returns more structured completion items, LSP. NFC.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/CodeComplete.cpp:259
+      : ASTCtx(ASTCtx), ExtractDocumentation(Opts.IncludeComments) {
+    if (C.SemaResult) {
+      Completion.Name = llvm::StringRef(SemaCCS->getTypedText());
----------------
ioeric wrote:
> nit: shall we keep `assert(bool(SemaResult) == bool(SemaCCS));`?
Done (in `add()`, and hoisted the call to `add()` to the top here)


================
Comment at: clangd/CodeComplete.cpp:322
+    }
+    if (ExtractDocumentation && Completion.Documentation.empty()) {
+      if (C.IndexResult && C.IndexResult->Detail)
----------------
ioeric wrote:
> nit: the documentation for the bundling says `Documentation may contain a combined docstring from all comments`. The implementation seems to take the documentation from the last comment (which seems fine to me), but they should be consistent.
This was meant to be ambiguous ("may") to allow a better implementation later. But rewrote it to mention the current behavior as a possibility :-)


================
Comment at: clangd/CodeComplete.cpp:1145
+    Scores.ExcludingName = Relevance.NameMatch
+                               ? Scores.Total / Relevance.NameMatch
+                               : Scores.Quality;
----------------
ioeric wrote:
> nit: this seems to assume that `NameMatch` is a factor of the final score, which seems to be changeable given the abstraction of `evaluateSymbolAndRelevance`. Not sure if there is an alternative to the current approach, but this probably deserves a comment.
Yeah, client-side filtering is the great abstraction-breaker :-(
Added the comment, not sure there's much more to be done.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48762