ilya-biryukov created this revision.
ilya-biryukov added reviewers: sammccall, hokein, ioeric.
Herald added subscribers: jkorous, MaskRay, klimek.
Previous implementation used to extract brief text from doxygen comments.
Brief text parsing slows down completion and is not suited for
non-doxygen comments.
This commit switches to providing comments that mimic the ones
originally written in the source code, doing minimal reindenting and
removing the comments markers to make the output more user-friendly.
It means we lose support for doxygen-specific features, e.g. extracting
brief text, but provide useful results for non-doxygen comments.
Switching the doxygen support back is an option, but I suggest to see
whether the current approach gives more useful results.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45999
Files:
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/CodeCompletionStrings.cpp
clangd/CodeCompletionStrings.h
clangd/index/SymbolCollector.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/CodeCompletionStringsTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===================================================================
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -490,11 +490,11 @@
}
)";
runSymbolCollector(Header, /*Main=*/"");
- EXPECT_THAT(Symbols,
- UnorderedElementsAre(QName("nx"),
- AllOf(QName("nx::ff"),
- Labeled("ff(int x, double y)"),
- Detail("int"), Doc("Foo comment."))));
+ EXPECT_THAT(
+ Symbols,
+ UnorderedElementsAre(
+ QName("nx"), AllOf(QName("nx::ff"), Labeled("ff(int x, double y)"),
+ Detail("int"), Doc("Foo comment."))));
}
TEST_F(SymbolCollectorTest, PlainAndSnippet) {
Index: unittests/clangd/CodeCompletionStringsTests.cpp
===================================================================
--- unittests/clangd/CodeCompletionStringsTests.cpp
+++ unittests/clangd/CodeCompletionStringsTests.cpp
@@ -51,23 +51,24 @@
}
TEST_F(CompletionStringTest, Documentation) {
- Builder.addBriefComment("Is this brief?");
- EXPECT_EQ(getDocumentation(*Builder.TakeString()), "Is this brief?");
+ Builder.addBriefComment("This is ignored");
+ EXPECT_EQ(getDocumentation(*Builder.TakeString(), "Is this brief?"),
+ "Is this brief?");
}
TEST_F(CompletionStringTest, DocumentationWithAnnotation) {
- Builder.addBriefComment("Is this brief?");
+ Builder.addBriefComment("This is ignored");
Builder.AddAnnotation("Ano");
- EXPECT_EQ(getDocumentation(*Builder.TakeString()),
+ EXPECT_EQ(getDocumentation(*Builder.TakeString(), "Is this brief?"),
"Annotation: Ano\n\nIs this brief?");
}
TEST_F(CompletionStringTest, MultipleAnnotations) {
Builder.AddAnnotation("Ano1");
Builder.AddAnnotation("Ano2");
Builder.AddAnnotation("Ano3");
- EXPECT_EQ(getDocumentation(*Builder.TakeString()),
+ EXPECT_EQ(getDocumentation(*Builder.TakeString(), ""),
"Annotations: Ano1 Ano2 Ano3\n");
}
@@ -97,7 +98,7 @@
TEST_F(CompletionStringTest, FunctionSnippet) {
Builder.AddResultTypeChunk("result no no");
- Builder.addBriefComment("Foo's comment");
+ Builder.addBriefComment("This comment is ignored");
Builder.AddTypedTextChunk("Foo");
Builder.AddChunk(CodeCompletionString::CK_LeftParen);
Builder.AddPlaceholderChunk("p1");
@@ -113,7 +114,7 @@
labelAndInsertText(*CCS, /*EnableSnippets=*/true);
EXPECT_EQ(Label, "Foo(p1, p2)");
EXPECT_EQ(InsertText, "Foo(${1:p1}, ${2:p2})");
- EXPECT_EQ(getDocumentation(*CCS), "Foo's comment");
+ EXPECT_EQ(getDocumentation(*CCS, "Foo's comment"), "Foo's comment");
EXPECT_EQ(getFilterText(*CCS), "Foo");
}
Index: unittests/clangd/CodeCompleteTests.cpp
===================================================================
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -236,8 +236,7 @@
// completion. At least there aren't any we're aware of.
EXPECT_THAT(Results.items, Not(Contains(Kind(CompletionItemKind::Snippet))));
// Check documentation.
- EXPECT_IFF(Opts.IncludeBriefComments, Results.items,
- Contains(IsDocumented()));
+ EXPECT_IFF(Opts.IncludeComments, Results.items, Contains(IsDocumented()));
}
void TestGlobalScopeCompletion(clangd::CodeCompleteOptions Opts) {
@@ -282,8 +281,7 @@
AllOf(Has("local_var"), Has("LocalClass"),
Contains(Kind(CompletionItemKind::Snippet))));
// Check documentation.
- EXPECT_IFF(Opts.IncludeBriefComments, Results.items,
- Contains(IsDocumented()));
+ EXPECT_IFF(Opts.IncludeComments, Results.items, Contains(IsDocumented()));
}
TEST(CompletionTest, CompletionOptions) {
@@ -294,7 +292,7 @@
// We used to test every combination of options, but that got too slow (2^N).
auto Flags = {
&clangd::CodeCompleteOptions::IncludeMacros,
- &clangd::CodeCompleteOptions::IncludeBriefComments,
+ &clangd::CodeCompleteOptions::IncludeComments,
&clangd::CodeCompleteOptions::EnableSnippets,
&clangd::CodeCompleteOptions::IncludeCodePatterns,
&clangd::CodeCompleteOptions::IncludeIneligibleResults,
Index: clangd/index/SymbolCollector.cpp
===================================================================
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -309,7 +309,8 @@
const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND,
SymbolID ID) {
- auto &SM = ND.getASTContext().getSourceManager();
+ auto &Ctx = ND.getASTContext();
+ auto &SM = Ctx.getSourceManager();
std::string QName;
llvm::raw_string_ostream OS(QName);
@@ -339,7 +340,7 @@
const auto *CCS = SymbolCompletion.CreateCodeCompletionString(
*ASTCtx, *PP, CodeCompletionContext::CCC_Name, *CompletionAllocator,
*CompletionTUInfo,
- /*IncludeBriefComments*/ true);
+ /*IncludeBriefComments*/ false);
std::string Label;
std::string SnippetInsertText;
std::string IgnoredLabel;
@@ -349,7 +350,8 @@
getLabelAndInsertText(*CCS, &IgnoredLabel, &PlainInsertText,
/*EnableSnippets=*/false);
std::string FilterText = getFilterText(*CCS);
- std::string Documentation = getDocumentation(*CCS);
+ std::string Documentation =
+ getDocumentation(*CCS, getDocComment(Ctx, SymbolCompletion));
std::string CompletionDetail = getDetail(*CCS);
std::string Include;
Index: clangd/CodeCompletionStrings.h
===================================================================
--- clangd/CodeCompletionStrings.h
+++ clangd/CodeCompletionStrings.h
@@ -17,8 +17,23 @@
#include "clang/Sema/CodeCompleteConsumer.h"
namespace clang {
+class ASTContext;
+
namespace clangd {
+/// Gets a raw documentation comment of \p Result.
+/// Returns empty string when no comment is available.
+std::string getDocComment(const ASTContext &Ctx,
+ const CodeCompletionResult &Result);
+
+/// Gets a raw documentation comment of the current active parameter
+/// of \p Result.
+/// Returns empty string when no comment is available.
+std::string
+getDocComment(const ASTContext &Ctx,
+ const CodeCompleteConsumer::OverloadCandidate &Result,
+ unsigned CurrentArg);
+
/// Gets label and insert text for a completion item. For example, for function
/// `Foo`, this returns <"Foo(int x, int y)", "Foo"> without snippts enabled.
///
@@ -29,7 +44,10 @@
/// Gets the documentation for a completion item. For example, comment for the
/// a class declaration.
-std::string getDocumentation(const CodeCompletionString &CCS);
+/// \p DocComment is a raw documentation comment that should be obtained via
+/// getDocComment functions declared above.
+std::string getDocumentation(const CodeCompletionString &CCS,
+ llvm::StringRef DocComment);
/// Gets detail to be used as the detail field in an LSP completion item. This
/// is usually the return type of a function.
Index: clangd/CodeCompletionStrings.cpp
===================================================================
--- clangd/CodeCompletionStrings.cpp
+++ clangd/CodeCompletionStrings.cpp
@@ -8,6 +8,7 @@
//===---------------------------------------------------------------------===//
#include "CodeCompletionStrings.h"
+#include "clang/AST/RawCommentList.h"
#include <utility>
namespace clang {
@@ -122,13 +123,42 @@
} // namespace
+std::string getDocComment(const ASTContext &Ctx,
+ const CodeCompletionResult &Result) {
+ // FIXME: clang's completion also returns documentation for RK_Pattern if they
+ // contain a pattern for ObjC properties. Unfortunately, there is no API to
+ // get this declaration, so we don't show documentation in that case.
+ if (Result.Kind != CodeCompletionResult::RK_Declaration)
+ return "";
+ auto Decl = Result.getDeclaration();
+ if (!Decl)
+ return "";
+ const RawComment *RC = getCompletionComment(Ctx, Decl);
+ if (!RC)
+ return "";
+ return RC->getFormattedText(Ctx);
+}
+
+std::string getDocComment(const ASTContext &Ctx,
+ const CodeCompleteConsumer::OverloadCandidate &Result,
+ unsigned CurrentArg) {
+ auto Func = Result.getFunction();
+ if (!Func)
+ return "";
+ const RawComment *RC = getParameterComment(Ctx, Result, CurrentArg);
+ if (!RC)
+ return "";
+ return RC->getFormattedText(Ctx);
+}
+
void getLabelAndInsertText(const CodeCompletionString &CCS, std::string *Label,
std::string *InsertText, bool EnableSnippets) {
return EnableSnippets ? processSnippetChunks(CCS, Label, InsertText)
: processPlainTextChunks(CCS, Label, InsertText);
}
-std::string getDocumentation(const CodeCompletionString &CCS) {
+std::string getDocumentation(const CodeCompletionString &CCS,
+ llvm::StringRef DocComment) {
// Things like __attribute__((nonnull(1,3))) and [[noreturn]]. Present this
// information in the documentation field.
std::string Result;
@@ -146,13 +176,13 @@
}
}
// Add brief documentation (if there is any).
- if (CCS.getBriefComment() != nullptr) {
+ if (!DocComment.empty()) {
if (!Result.empty()) {
// This means we previously added annotations. Add an extra newline
// character to make the annotations stand out.
Result.push_back('\n');
}
- Result += CCS.getBriefComment();
+ Result += DocComment;
}
return Result;
}
Index: clangd/CodeComplete.h
===================================================================
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -44,10 +44,8 @@
/// Add macros to code completion results.
bool IncludeMacros = true;
- /// Add brief comments to completion items, if available.
- /// FIXME(ibiryukov): it looks like turning this option on significantly slows
- /// down completion, investigate if it can be made faster.
- bool IncludeBriefComments = true;
+ /// Add comments to code completion results, if available.
+ bool IncludeComments = true;
/// Include results that are not legal completions in the current context.
/// For example, private members are usually inaccessible.
Index: clangd/CodeComplete.cpp
===================================================================
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -255,15 +255,16 @@
CompletionItem build(llvm::StringRef FileName,
const CompletionItemScores &Scores,
const CodeCompleteOptions &Opts,
- CodeCompletionString *SemaCCS) const {
+ CodeCompletionString *SemaCCS,
+ llvm::StringRef DocComment) const {
assert(bool(SemaResult) == bool(SemaCCS));
CompletionItem I;
if (SemaResult) {
I.kind = toCompletionItemKind(SemaResult->Kind, SemaResult->CursorKind);
getLabelAndInsertText(*SemaCCS, &I.label, &I.insertText,
Opts.EnableSnippets);
I.filterText = getFilterText(*SemaCCS);
- I.documentation = getDocumentation(*SemaCCS);
+ I.documentation = getDocumentation(*SemaCCS, DocComment);
I.detail = getDetail(*SemaCCS);
}
if (IndexResult) {
@@ -502,17 +503,17 @@
case CodeCompletionResult::RK_Pattern:
return Result.Pattern->getTypedText();
}
- auto *CCS = codeCompletionString(Result, /*IncludeBriefComments=*/false);
+ auto *CCS = codeCompletionString(Result);
return CCS->getTypedText();
}
// Build a CodeCompletion string for R, which must be from Results.
// The CCS will be owned by this recorder.
- CodeCompletionString *codeCompletionString(const CodeCompletionResult &R,
- bool IncludeBriefComments) {
+ CodeCompletionString *codeCompletionString(const CodeCompletionResult &R) {
// CodeCompletionResult doesn't seem to be const-correct. We own it, anyway.
return const_cast<CodeCompletionResult &>(R).CreateCodeCompletionString(
- *CCSema, CCContext, *CCAllocator, CCTUInfo, IncludeBriefComments);
+ *CCSema, CCContext, *CCAllocator, CCTUInfo,
+ /*IncludeBriefComments=*/false);
}
private:
@@ -594,7 +595,9 @@
const auto *CCS = Candidate.CreateSignatureString(
CurrentArg, S, *Allocator, CCTUInfo, true);
assert(CCS && "Expected the CodeCompletionString to be non-null");
- SigHelp.signatures.push_back(ProcessOverloadCandidate(Candidate, *CCS));
+ SigHelp.signatures.push_back(ProcessOverloadCandidate(
+ Candidate, *CCS,
+ getDocComment(S.getASTContext(), Candidate, CurrentArg)));
}
}
@@ -607,11 +610,12 @@
// CompletionString.h.
SignatureInformation
ProcessOverloadCandidate(const OverloadCandidate &Candidate,
- const CodeCompletionString &CCS) const {
+ const CodeCompletionString &CCS,
+ llvm::StringRef DocComment) const {
SignatureInformation Result;
const char *ReturnType = nullptr;
- Result.documentation = getDocumentation(CCS);
+ Result.documentation = getDocumentation(CCS, DocComment);
for (const auto &Chunk : CCS) {
switch (Chunk.Kind) {
@@ -800,7 +804,9 @@
Result.IncludeCodePatterns = EnableSnippets && IncludeCodePatterns;
Result.IncludeMacros = IncludeMacros;
Result.IncludeGlobals = true;
- Result.IncludeBriefComments = IncludeBriefComments;
+ // We never include brief comments, they are slow and don't provide useful
+ // results for non-doxygen comments.
+ Result.IncludeBriefComments = false;
// When an is used, Sema is responsible for completing the main file,
// the index can provide results from the preamble.
@@ -992,9 +998,15 @@
CompletionItem toCompletionItem(const CompletionCandidate &Candidate,
const CompletionItemScores &Scores) {
CodeCompletionString *SemaCCS = nullptr;
- if (auto *SR = Candidate.SemaResult)
- SemaCCS = Recorder->codeCompletionString(*SR, Opts.IncludeBriefComments);
- return Candidate.build(FileName, Scores, Opts, SemaCCS);
+ std::string DocComment;
+ if (auto *SR = Candidate.SemaResult) {
+ SemaCCS = Recorder->codeCompletionString(*SR);
+ if (Opts.IncludeComments) {
+ assert(Recorder->CCSema);
+ DocComment = getDocComment(Recorder->CCSema->getASTContext(), *SR);
+ }
+ }
+ return Candidate.build(FileName, Scores, Opts, SemaCCS, DocComment);
}
};
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
