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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits