[clang-tools-extra] b061564 - [clangd] Make our printing policies for Hover more consistent, especially tags
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 18 16:03:15 PST 2020
Author: Sam McCall
Date: 2020-12-19T00:52:55+01:00
New Revision: b0615642f647bea1483659f1e14515a836015254
URL: https://github.com/llvm/llvm-project/commit/b0615642f647bea1483659f1e14515a836015254
DIFF: https://github.com/llvm/llvm-project/commit/b0615642f647bea1483659f1e14515a836015254.diff
LOG: [clangd] Make our printing policies for Hover more consistent, especially tags
Different cases were using a bunch of different variants of the printing policy.
Each of these had something going for it, but the result was inconsistent.
Goals:
- single printing policy used (almost) everywhere
- avoid unidiomatic tags like `class vector<class X>`
- be informative and easy to understand
For tags, the solution I wound up with is: we print only the outer tag and only
in the simplest cases where this elaboration won't cause confusion.
For example:
- class X
- enum Foo
- vector<int>
- X*
This seems to strike a nice balance of providing plenty of info/context in common
cases while never being confusing.
Differential Revision: https://reviews.llvm.org/D93553
Added:
Modified:
clang-tools-extra/clangd/Hover.cpp
clang-tools-extra/clangd/unittests/HoverTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp
index b5eda93ddbbc..6d707c8d1521 100644
--- a/clang-tools-extra/clangd/Hover.cpp
+++ b/clang-tools-extra/clangd/Hover.cpp
@@ -49,16 +49,13 @@ namespace clang {
namespace clangd {
namespace {
-PrintingPolicy printingPolicyForDecls(PrintingPolicy Base) {
- PrintingPolicy Policy(Base);
-
- Policy.AnonymousTagLocations = false;
- Policy.TerseOutput = true;
- Policy.PolishForDeclaration = true;
- Policy.ConstantsAsWritten = true;
- Policy.SuppressTagKeyword = false;
-
- return Policy;
+PrintingPolicy getPrintingPolicy(PrintingPolicy Base) {
+ Base.AnonymousTagLocations = false;
+ Base.TerseOutput = true;
+ Base.PolishForDeclaration = true;
+ Base.ConstantsAsWritten = true;
+ Base.SuppressTemplateArgsInCXXConstructors = true;
+ return Base;
}
/// Given a declaration \p D, return a human-readable string representing the
@@ -108,26 +105,33 @@ std::string getNamespaceScope(const Decl *D) {
return "";
}
-std::string printDefinition(const Decl *D) {
+std::string printDefinition(const Decl *D, const PrintingPolicy &PP) {
std::string Definition;
llvm::raw_string_ostream OS(Definition);
- PrintingPolicy Policy =
- printingPolicyForDecls(D->getASTContext().getPrintingPolicy());
- Policy.IncludeTagDefinition = false;
- Policy.SuppressTemplateArgsInCXXConstructors = true;
- Policy.SuppressTagKeyword = true;
- D->print(OS, Policy);
+ D->print(OS, PP);
OS.flush();
return Definition;
}
-std::string printType(QualType QT, const PrintingPolicy &Policy) {
+std::string printType(QualType QT, const PrintingPolicy &PP) {
// TypePrinter doesn't resolve decltypes, so resolve them here.
// FIXME: This doesn't handle composite types that contain a decltype in them.
// We should rather have a printing policy for that.
while (!QT.isNull() && QT->isDecltypeType())
QT = QT->getAs<DecltypeType>()->getUnderlyingType();
- return QT.getAsString(Policy);
+ std::string Result;
+ llvm::raw_string_ostream OS(Result);
+ // Special case: if the outer type is a tag type without qualifiers, then
+ // include the tag for extra clarity.
+ // This isn't very idiomatic, so don't attempt it for complex cases, including
+ // pointers/references, template specializations, etc.
+ if (!QT.isNull() && !QT.hasQualifiers() && PP.SuppressTagKeyword) {
+ if (auto *TT = llvm::dyn_cast<TagType>(QT.getTypePtr()))
+ OS << TT->getDecl()->getKindName() << " ";
+ }
+ OS.flush();
+ QT.print(OS, PP);
+ return Result;
}
std::string printType(const TemplateTypeParmDecl *TTP) {
@@ -291,15 +295,15 @@ const Expr *getDefaultArg(const ParmVarDecl *PVD) {
}
HoverInfo::Param toHoverInfoParam(const ParmVarDecl *PVD,
- const PrintingPolicy &Policy) {
+ const PrintingPolicy &PP) {
HoverInfo::Param Out;
- Out.Type = printType(PVD->getType(), Policy);
+ Out.Type = printType(PVD->getType(), PP);
if (!PVD->getName().empty())
Out.Name = PVD->getNameAsString();
if (const Expr *DefArg = getDefaultArg(PVD)) {
Out.Default.emplace();
llvm::raw_string_ostream OS(*Out.Default);
- DefArg->printPretty(OS, nullptr, Policy);
+ DefArg->printPretty(OS, nullptr, PP);
}
return Out;
}
@@ -307,10 +311,10 @@ HoverInfo::Param toHoverInfoParam(const ParmVarDecl *PVD,
// Populates Type, ReturnType, and Parameters for function-like decls.
void fillFunctionTypeAndParams(HoverInfo &HI, const Decl *D,
const FunctionDecl *FD,
- const PrintingPolicy &Policy) {
+ const PrintingPolicy &PP) {
HI.Parameters.emplace();
for (const ParmVarDecl *PVD : FD->parameters())
- HI.Parameters->emplace_back(toHoverInfoParam(PVD, Policy));
+ HI.Parameters->emplace_back(toHoverInfoParam(PVD, PP));
// We don't want any type info, if name already contains it. This is true for
// constructors/destructors and conversion operators.
@@ -320,11 +324,11 @@ void fillFunctionTypeAndParams(HoverInfo &HI, const Decl *D,
NK == DeclarationName::CXXConversionFunctionName)
return;
- HI.ReturnType = printType(FD->getReturnType(), Policy);
+ HI.ReturnType = printType(FD->getReturnType(), PP);
QualType QT = FD->getType();
if (const VarDecl *VD = llvm::dyn_cast<VarDecl>(D)) // Lambdas
QT = VD->getType().getDesugaredType(D->getASTContext());
- HI.Type = printType(QT, Policy);
+ HI.Type = printType(QT, PP);
// FIXME: handle variadics.
}
@@ -492,7 +496,8 @@ std::string synthesizeDocumentation(const NamedDecl *ND) {
}
/// Generate a \p Hover object given the declaration \p D.
-HoverInfo getHoverContents(const NamedDecl *D, const SymbolIndex *Index) {
+HoverInfo getHoverContents(const NamedDecl *D, const PrintingPolicy &PP,
+ const SymbolIndex *Index) {
HoverInfo HI;
const ASTContext &Ctx = D->getASTContext();
@@ -504,7 +509,6 @@ HoverInfo getHoverContents(const NamedDecl *D, const SymbolIndex *Index) {
if (!HI.LocalScope.empty())
HI.LocalScope.append("::");
- PrintingPolicy Policy = printingPolicyForDecls(Ctx.getPrintingPolicy());
HI.Name = printName(Ctx, *D);
const auto *CommentD = getDeclForComment(D);
HI.Documentation = getDeclComment(Ctx, *CommentD);
@@ -517,25 +521,25 @@ HoverInfo getHoverContents(const NamedDecl *D, const SymbolIndex *Index) {
// Fill in template params.
if (const TemplateDecl *TD = D->getDescribedTemplate()) {
HI.TemplateParameters =
- fetchTemplateParameters(TD->getTemplateParameters(), Policy);
+ fetchTemplateParameters(TD->getTemplateParameters(), PP);
D = TD;
} else if (const FunctionDecl *FD = D->getAsFunction()) {
if (const auto *FTD = FD->getDescribedTemplate()) {
HI.TemplateParameters =
- fetchTemplateParameters(FTD->getTemplateParameters(), Policy);
+ fetchTemplateParameters(FTD->getTemplateParameters(), PP);
D = FTD;
}
}
// Fill in types and params.
if (const FunctionDecl *FD = getUnderlyingFunction(D))
- fillFunctionTypeAndParams(HI, D, FD, Policy);
+ fillFunctionTypeAndParams(HI, D, FD, PP);
else if (const auto *VD = dyn_cast<ValueDecl>(D))
- HI.Type = printType(VD->getType(), Policy);
+ HI.Type = printType(VD->getType(), PP);
else if (const auto *TTP = dyn_cast<TemplateTypeParmDecl>(D))
HI.Type = TTP->wasDeclaredWithTypename() ? "typename" : "class";
else if (const auto *TTP = dyn_cast<TemplateTemplateParmDecl>(D))
- HI.Type = printType(TTP, Policy);
+ HI.Type = printType(TTP, PP);
// Fill in value with evaluated initializer if possible.
if (const auto *Var = dyn_cast<VarDecl>(D)) {
@@ -547,7 +551,7 @@ HoverInfo getHoverContents(const NamedDecl *D, const SymbolIndex *Index) {
HI.Value = ECD->getInitVal().toString(10);
}
- HI.Definition = printDefinition(D);
+ HI.Definition = printDefinition(D, PP);
return HI;
}
@@ -587,7 +591,8 @@ HoverInfo getHoverContents(const DefinedMacro &Macro, ParsedAST &AST) {
}
llvm::Optional<HoverInfo> getThisExprHoverContents(const CXXThisExpr *CTE,
- ASTContext &ASTCtx) {
+ ASTContext &ASTCtx,
+ const PrintingPolicy &PP) {
QualType OriginThisType = CTE->getType()->getPointeeType();
QualType ClassType = declaredType(OriginThisType->getAsTagDecl());
// For partial specialization class, origin `this` pointee type will be
@@ -597,30 +602,26 @@ llvm::Optional<HoverInfo> getThisExprHoverContents(const CXXThisExpr *CTE,
QualType PrettyThisType = ASTCtx.getPointerType(
QualType(ClassType.getTypePtr(), OriginThisType.getCVRQualifiers()));
- auto Policy = printingPolicyForDecls(ASTCtx.getPrintingPolicy());
- Policy.SuppressTagKeyword = true;
- Policy.SuppressScope = true;
HoverInfo HI;
HI.Name = "this";
- HI.Definition = PrettyThisType.getAsString(Policy);
+ HI.Definition = printType(PrettyThisType, PP);
return HI;
}
/// Generate a HoverInfo object given the deduced type \p QT
HoverInfo getDeducedTypeHoverContents(QualType QT, const syntax::Token &Tok,
ASTContext &ASTCtx,
+ const PrintingPolicy &PP,
const SymbolIndex *Index) {
HoverInfo HI;
// FIXME: distinguish decltype(auto) vs decltype(expr)
HI.Name = tok::getTokenName(Tok.kind());
HI.Kind = index::SymbolKind::TypeAlias;
- auto PP = printingPolicyForDecls(ASTCtx.getLangOpts());
-
if (QT->isUndeducedAutoType()) {
HI.Definition = "/* not deduced */";
} else {
- HI.Definition = QT.getAsString(PP);
+ HI.Definition = printType(QT, PP);
if (const auto *D = QT->getAsTagDecl()) {
const auto *CommentD = getDeclForComment(D);
@@ -657,6 +658,7 @@ llvm::StringLiteral getNameForExpr(const Expr *E) {
// Generates hover info for `this` and evaluatable expressions.
// FIXME: Support hover for literals (esp user-defined)
llvm::Optional<HoverInfo> getHoverContents(const Expr *E, ParsedAST &AST,
+ const PrintingPolicy &PP,
const SymbolIndex *Index) {
// There's not much value in hovering over "42" and getting a hover card
// saying "42 is an int", similar for other literals.
@@ -666,14 +668,11 @@ llvm::Optional<HoverInfo> getHoverContents(const Expr *E, ParsedAST &AST,
HoverInfo HI;
// For `this` expr we currently generate hover with pointee type.
if (const CXXThisExpr *CTE = dyn_cast<CXXThisExpr>(E))
- return getThisExprHoverContents(CTE, AST.getASTContext());
+ return getThisExprHoverContents(CTE, AST.getASTContext(), PP);
// For expressions we currently print the type and the value, iff it is
// evaluatable.
if (auto Val = printExprValue(E, AST.getASTContext())) {
- auto Policy =
- printingPolicyForDecls(AST.getASTContext().getPrintingPolicy());
- Policy.SuppressTagKeyword = true;
- HI.Type = printType(E->getType(), Policy);
+ HI.Type = printType(E->getType(), PP);
HI.Value = *Val;
HI.Name = std::string(getNameForExpr(E));
return HI;
@@ -744,7 +743,7 @@ void addLayoutInfo(const NamedDecl &ND, HoverInfo &HI) {
// If N is passed as argument to a function, fill HI.CalleeArgInfo with
// information about that argument.
void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo &HI,
- const PrintingPolicy &Policy) {
+ const PrintingPolicy &PP) {
const auto &OuterNode = N->outerImplicit();
if (!OuterNode.Parent)
return;
@@ -768,7 +767,7 @@ void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo &HI,
// Extract matching argument from function declaration.
if (const ParmVarDecl *PVD = FD->getParamDecl(I))
- HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, Policy));
+ HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, PP));
break;
}
if (!HI.CalleeArgInfo)
@@ -832,6 +831,8 @@ void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo &HI,
llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
format::FormatStyle Style,
const SymbolIndex *Index) {
+ PrintingPolicy PP =
+ getPrintingPolicy(AST.getASTContext().getPrintingPolicy());
const SourceManager &SM = AST.getSourceManager();
auto CurLoc = sourceLocationInMainFile(SM, Pos);
if (!CurLoc) {
@@ -863,7 +864,7 @@ llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
}
} else if (Tok.kind() == tok::kw_auto || Tok.kind() == tok::kw_decltype) {
if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) {
- HI = getDeducedTypeHoverContents(*Deduced, Tok, AST.getASTContext(),
+ HI = getDeducedTypeHoverContents(*Deduced, Tok, AST.getASTContext(), PP,
Index);
HighlightRange = Tok.range(SM).toCharRange(SM);
break;
@@ -888,16 +889,16 @@ llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
// FIXME: Fill in HighlightRange with range coming from N->ASTNode.
auto Decls = explicitReferenceTargets(N->ASTNode, DeclRelation::Alias);
if (!Decls.empty()) {
- HI = getHoverContents(Decls.front(), Index);
+ HI = getHoverContents(Decls.front(), PP, Index);
// Layout info only shown when hovering on the field/class itself.
if (Decls.front() == N->ASTNode.get<Decl>())
addLayoutInfo(*Decls.front(), *HI);
// Look for a close enclosing expression to show the value of.
if (!HI->Value)
HI->Value = printExprValue(N, AST.getASTContext());
- maybeAddCalleeArgInfo(N, *HI, AST.getASTContext().getPrintingPolicy());
+ maybeAddCalleeArgInfo(N, *HI, PP);
} else if (const Expr *E = N->ASTNode.get<Expr>()) {
- HI = getHoverContents(E, AST, Index);
+ HI = getHoverContents(E, AST, PP, Index);
}
// FIXME: support hovers for other nodes?
// - built-in types
diff --git a/clang-tools-extra/clangd/unittests/HoverTests.cpp b/clang-tools-extra/clangd/unittests/HoverTests.cpp
index b72b500ee8b5..9650940b66cb 100644
--- a/clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ b/clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -239,7 +239,7 @@ class Foo {})cpp";
HI.Name = "c";
HI.Kind = index::SymbolKind::Variable;
HI.Definition = "auto *c = &b";
- HI.Type = "class (lambda) **";
+ HI.Type = "(lambda) **";
HI.ReturnType = "bool";
HI.Parameters = {
{std::string("int"), std::string("T"), llvm::None},
@@ -611,7 +611,7 @@ class Foo {})cpp";
[](HoverInfo &HI) {
HI.Name = "auto";
HI.Kind = index::SymbolKind::TypeAlias;
- HI.Definition = "class Foo<class X>";
+ HI.Definition = "class Foo<X>";
}},
{// Falls back to primary template, when the type is not instantiated.
R"cpp(
@@ -718,8 +718,8 @@ class Foo {})cpp";
HI.Definition = "X &setY(float v)";
HI.LocalScope = "X::";
HI.Documentation = "Trivial setter for `Y`.";
- HI.Type = "struct X &(float)";
- HI.ReturnType = "struct X &";
+ HI.Type = "X &(float)";
+ HI.ReturnType = "X &";
HI.Parameters.emplace();
HI.Parameters->emplace_back();
HI.Parameters->back().Type = "float";
@@ -1943,7 +1943,7 @@ TEST(Hover, All) {
HI.Kind = index::SymbolKind::Variable;
HI.NamespaceScope = "";
HI.LocalScope = "test::";
- HI.Type = "struct Test &&";
+ HI.Type = "Test &&";
HI.Definition = "Test &&test = {}";
}},
{
@@ -2211,7 +2211,7 @@ TEST(Hover, All) {
)cpp",
[](HoverInfo &HI) {
HI.Name = "this";
- HI.Definition = "Foo *";
+ HI.Definition = "ns::Foo *";
}},
{
R"cpp(// this expr for template class
More information about the cfe-commits
mailing list