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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Looks great! Mostly nits.



================
Comment at: clangd/CodeComplete.cpp:259
+      : ASTCtx(ASTCtx), ExtractDocumentation(Opts.IncludeComments) {
+    if (C.SemaResult) {
+      Completion.Name = llvm::StringRef(SemaCCS->getTypedText());
----------------
nit: shall we keep `assert(bool(SemaResult) == bool(SemaCCS));`?


================
Comment at: clangd/CodeComplete.cpp:279
+    if (auto Inserted = C.headerToInsertIfNotPresent()) {
+      llvm::errs() << "Inserted = " << *Inserted << "\n";
+      Completion.Header = *Inserted;
----------------
remove?


================
Comment at: clangd/CodeComplete.cpp:280
+      llvm::errs() << "Inserted = " << *Inserted << "\n";
+      Completion.Header = *Inserted;
+      // Turn absolute path into a literal string that can be #included.
----------------
`Inserted` can be an absolute path which might have long uninteresting prefix.  It might make sense to use the result from `calculateIncludePath`? 


================
Comment at: clangd/CodeComplete.cpp:309
+
+  void add(const CompletionCandidate &C, CodeCompletionString *SemaCCS) {
+    Bundled.emplace_back();
----------------
nit: It's unclear why `add` is a public interface given that the constructor also adds candidate. Might worth a comment. Also, it seems that the interface design has been driven by by the bundling logic, so it might also make sense to explain a bit on the class level.


================
Comment at: clangd/CodeComplete.cpp:322
+    }
+    if (ExtractDocumentation && Completion.Documentation.empty()) {
+      if (C.IndexResult && C.IndexResult->Detail)
----------------
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.


================
Comment at: clangd/CodeComplete.cpp:346
+
+  template <std::string BundledEntry::*Member>
+  const std::string *onlyValue() const {
----------------
nit: this might worth a comment. it's unclear what this does by just looking at the name.


================
Comment at: clangd/CodeComplete.cpp:1145
+    Scores.ExcludingName = Relevance.NameMatch
+                               ? Scores.Total / Relevance.NameMatch
+                               : Scores.Quality;
----------------
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.


================
Comment at: clangd/CodeComplete.cpp:1161
 
-  CompletionItem toCompletionItem(const CompletionCandidate::Bundle &Bundle,
-                                  const CompletionItemScores &Scores) {
-    CodeCompletionString *SemaCCS = nullptr;
-    std::string FrontDocComment;
-    if (auto *SR = Bundle.front().SemaResult) {
-      SemaCCS = Recorder->codeCompletionString(*SR);
-      if (Opts.IncludeComments) {
-        assert(Recorder->CCSema);
-        FrontDocComment = getDocComment(Recorder->CCSema->getASTContext(), *SR,
-                                        /*CommentsFromHeader=*/false);
-      }
+  CodeCompletion toCompletionItem(const CompletionCandidate::Bundle &Bundle) {
+    llvm::Optional<CodeCompletionBuilder> Builder;
----------------
nit: toCompletionItem might be ambiguous now that this returns `CodeCompletion`.


================
Comment at: clangd/CodeComplete.h:88
+  // Qualifiers that must be inserted before the name (e.g. base::).
+  std::string Qualifiers;
+  // Details to be displayed following the name. Not inserted.
----------------
Give that this field has caused confusion before, would it make sense to make the name a bit more concrete like `RequiredQualifier` or `InsertedQualifier`?


================
Comment at: clangd/CodeComplete.h:106
+  unsigned BundleSize;
+  // The header through which this symbol should be included.
+  // Empty for non-symbol completions.
----------------
Would this be an absolute path or an include path?


================
Comment at: clangd/CodeComplete.h:140
+  std::vector<CodeCompletion> Completions;
+  bool More = false;
+};
----------------
I found `Result.More` a bit not straightforward. Maybe `HasMore`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48762