upsj updated this revision to Diff 427928.
upsj added a comment.
- add test for emplace-like functions
- fix RecursiveASTVisitor early exit
- fix handling of skipped parameters
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124690/new/
https://reviews.llvm.org/D124690
Files:
clang-tools-extra/clangd/InlayHints.cpp
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -170,6 +170,43 @@
)cpp");
}
+TEST(ParameterHints, NoNameVariadicDeclaration) {
+ // No hint for anonymous variadic parameter
+ assertParameterHints(R"cpp(
+ template <typename... Args>
+ void foo(Args&& ...);
+ void bar() {
+ foo(42);
+ }
+ )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicForwarded) {
+ // No hint for anonymous variadic parameter
+ // The forward prototype is not correct, but is converted into builtin anyways
+ assertParameterHints(R"cpp(
+ namespace std { template <typename T> T&& forward(T&); }
+ void foo(int);
+ template <typename... Args>
+ void bar(Args&&... args) { return foo(std::forward<Args>(args)...); }
+ void baz() {
+ bar(42);
+ }
+ )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+ // No hint for anonymous variadic parameter
+ assertParameterHints(R"cpp(
+ void foo(int);
+ template <typename... Args>
+ void bar(Args&&... args) { return foo(args...); }
+ void baz() {
+ bar(42);
+ }
+ )cpp");
+}
+
TEST(ParameterHints, NameInDefinition) {
// Parameter name picked up from definition if necessary.
assertParameterHints(R"cpp(
@@ -182,6 +219,19 @@
ExpectedHint{"param: ", "param"});
}
+TEST(ParameterHints, NamePartiallyInDefinition) {
+ // Parameter name picked up from definition if necessary.
+ assertParameterHints(R"cpp(
+ void foo(int, int b);
+ void bar() {
+ foo($param1[[42]], $param2[[42]]);
+ }
+ void foo(int a, int) {};
+ )cpp",
+ ExpectedHint{"a: ", "param1"},
+ ExpectedHint{"b: ", "param2"});
+}
+
TEST(ParameterHints, NameMismatch) {
// Prefer name from declaration.
assertParameterHints(R"cpp(
@@ -254,6 +304,206 @@
ExpectedHint{"param: ", "param"});
}
+TEST(ParameterHints, VariadicForwardedConstructor) {
+ // Name hint for variadic parameter
+ // The forward prototype is not correct, but is converted into builtin anyways
+ assertParameterHints(R"cpp(
+ namespace std { template <typename T> T&& forward(T&); }
+ struct S { S(int a); };
+ template <typename T, typename... Args>
+ T bar(Args&&... args) { return T{std::forward<Args>(args)...}; }
+ void baz() {
+ int b;
+ bar<S>($param[[b]]);
+ }
+ )cpp",
+ ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+ // Name hint for variadic parameter
+ assertParameterHints(R"cpp(
+ struct S { S(int a); };
+ template <typename T, typename... Args>
+ T bar(Args&&... args) { return T{args...}; }
+ void baz() {
+ int b;
+ bar<S>($param[[b]]);
+ }
+ )cpp",
+ ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwardedNewConstructor) {
+ // Name hint for variadic parameter
+ // The forward prototype is not correct, but is converted into builtin anyways
+ assertParameterHints(R"cpp(
+ namespace std { template <typename T> T&& forward(T&); }
+ struct S { S(int a); };
+ template <typename T, typename... Args>
+ T* bar(Args&&... args) { return new T{std::forward<Args>(args)...}; }
+ void baz() {
+ int b;
+ bar<S>($param[[b]]);
+ }
+ )cpp",
+ ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+ // Name hint for variadic parameter
+ assertParameterHints(R"cpp(
+ struct S { S(int a); };
+ template <typename T, typename... Args>
+ T* bar(Args&&... args) { return new T{args...}; }
+ void baz() {
+ int b;
+ bar<S>($param[[b]]);
+ }
+ )cpp",
+ ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwarded) {
+ // Name for variadic parameter
+ // The forward prototype is not correct, but is converted into builtin anyways
+ assertParameterHints(R"cpp(
+ namespace std { template <typename T> T&& forward(T&); }
+ void foo(int a);
+ template <typename... Args>
+ void bar(Args&&... args) { return foo(std::forward<Args>(args)...); }
+ void baz() {
+ int b;
+ bar($param[[b]]);
+ }
+ )cpp",
+ ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlain) {
+ // Name hint for variadic parameter
+ assertParameterHints(R"cpp(
+ void foo(int a);
+ template <typename... Args>
+ void bar(Args&&... args) { return foo(args...); }
+ void baz() {
+ bar($param[[42]]);
+ }
+ )cpp",
+ ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicSplitRecursive) {
+ // Name for variadic parameter
+ // The forward prototype is not correct, but is converted into builtin anyways
+ assertParameterHints(R"cpp(
+ namespace std { template <typename T> T&& forward(T&); }
+ void baz(int b);
+ template <typename... Args>
+ void foo(int a, Args&&... args) {
+ return baz(std::forward<Args>(args)...);
+ }
+ template <typename... Args>
+ void bar(Args&&... args) { return foo(std::forward<Args>(args)...); }
+ void bazz() {
+ bar($param1[[32]], $param2[[42]]);
+ }
+ )cpp",
+ ExpectedHint{"a: ", "param1"},
+ ExpectedHint{"b: ", "param2"});
+}
+
+TEST(ParameterHints, VariadicOverloaded) {
+ // Name for variadic parameter
+ // The forward prototype is not correct, but is converted into builtin anyways
+ assertParameterHints(
+ R"cpp(
+ namespace std { template <typename T> T&& forward(T&); }
+ void baz(int b, int c);
+ void baz(int bb, int cc, int dd);
+ template <typename... Args>
+ void foo(int a, Args&&... args) {
+ return baz(std::forward<Args>(args)...);
+ }
+ template <typename... Args>
+ void bar(Args&&... args) { return foo(std::forward<Args>(args)...); }
+ void bazz() {
+ bar($param1[[32]], $param2[[42]], $param3[[52]]);
+ bar($param4[[1]], $param5[[2]], $param6[[3]], $param7[[4]]);
+ }
+ )cpp",
+ ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
+ ExpectedHint{"c: ", "param3"}, ExpectedHint{"a: ", "param4"},
+ ExpectedHint{"bb: ", "param5"}, ExpectedHint{"cc: ", "param6"},
+ ExpectedHint{"dd: ", "param7"});
+}
+
+TEST(ParameterHints, VariadicEmplace) {
+ // Name for variadic parameter
+ // The forward prototype is not correct, but is converted into builtin anyways
+ assertParameterHints(
+ R"cpp(
+ namespace std { template <typename T> T&& forward(T&); }
+ void *operator new(unsigned long, void *);
+ struct S {
+ S(int A);
+ S(int B, int C);
+ };
+ struct alloc {
+ template <typename T>
+ T* allocate();
+ template <typename T, typename... Args>
+ void construct(T* ptr, Args&&... args) {
+ ::new ((void*)ptr) T{std::forward<Args>(args)...};
+ }
+ };
+ template <typename T>
+ struct container {
+ template <typename... Args>
+ void emplace(Args&&... args) {
+ alloc a;
+ auto ptr = a.template allocate<T>();
+ a.construct(ptr, std::forward<Args>(args)...);
+ }
+ };
+ void foo() {
+ container<S> c;
+ c.emplace($param1[[1]]);
+ c.emplace($param2[[2]], $param3[[3]]);
+ }
+ )cpp",
+ ExpectedHint{"A: ", "param1"}, ExpectedHint{"B: ", "param2"},
+ ExpectedHint{"C: ", "param3"});
+}
+
+TEST(ParameterHints, MatchingNameVariadicForwarded) {
+ // No name hint for variadic parameter with matching name
+ // The forward prototype is not correct, but is converted into builtin anyways
+ assertParameterHints(R"cpp(
+ namespace std { template <typename T> T&& forward(T&); }
+ void foo(int a);
+ template <typename... Args>
+ void bar(Args&&... args) { return foo(std::forward<Args>(args)...); }
+ void baz() {
+ int a;
+ bar(a);
+ }
+ )cpp");
+}
+
+TEST(ParameterHints, MatchingNameVariadicPlain) {
+ // No name hint for variadic parameter with matching name
+ assertParameterHints(R"cpp(
+ void foo(int a);
+ template <typename... Args>
+ void bar(Args&&... args) { return foo(args...); }
+ void baz() {
+ int a;
+ bar(a);
+ }
+ )cpp");
+}
+
TEST(ParameterHints, Operator) {
// No hint for operator call with operator syntax.
assertParameterHints(R"cpp(
Index: clang-tools-extra/clangd/InlayHints.cpp
===================================================================
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -14,6 +14,7 @@
#include "clang/AST/DeclarationName.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Basic/Builtins.h"
#include "clang/Basic/SourceManager.h"
#include "llvm/ADT/ScopeExit.h"
@@ -185,6 +186,194 @@
return Designators;
}
+bool isExpandedParameter(const ParmVarDecl *Param) {
+ auto PlainType = Param->getType().getNonReferenceType();
+ if (const auto *SubstType = dyn_cast<SubstTemplateTypeParmType>(PlainType)) {
+ return SubstType->getReplacedParameter()->isParameterPack();
+ }
+ return false;
+}
+
+class ForwardingParameterVisitor
+ : public RecursiveASTVisitor<ForwardingParameterVisitor> {
+public:
+ void
+ resolveForwardedParameters(const FunctionDecl *Callee,
+ llvm::SmallVector<const ParmVarDecl *> &Params) {
+ if (!TraversedFunctions.contains(Callee)) {
+ UnresolvedParams.clear();
+ size_t ParamIdx = 0;
+ for (const auto *Param : Params) {
+ // If the parameter is part of an expanded pack and not yet resolved
+ if (/*isExpandedParameter(Param) && */
+ ForwardedParams.find(Param) == ForwardedParams.end()) {
+ UnresolvedParams.insert(Param);
+ }
+ handleParmVarDeclName(Callee, ParamIdx);
+ ParamIdx++;
+ }
+ // Recursively resolve forwarded parameters
+ if (!UnresolvedParams.empty()) {
+ TraversalQueue.push_back(const_cast<FunctionDecl *>(Callee));
+ while (!TraversalQueue.empty()) {
+ auto *CurrentFunction = TraversalQueue.front();
+ TraversedFunctions.insert(CurrentFunction);
+ TraverseFunctionDecl(CurrentFunction);
+ TraversalQueue.pop_front();
+ }
+ }
+ }
+ // Replace parameters by their forwarded counterpart
+ for (auto &Param : Params) {
+ Param = getForwardedParameter(Param);
+ Param = getNamedParameter(Param);
+ }
+ }
+
+ bool VisitCXXConstructExpr(CXXConstructExpr *E) {
+ // if there are no va_args, we can forward parameters
+ if (E->getConstructor()->getNumParams() == E->getNumArgs()) {
+ handleCallOrConstructExpr(E->getConstructor(), E->arguments());
+ }
+ // continue searching only if there are unresolved parameters left.
+ return !UnresolvedParams.empty();
+ }
+
+ bool VisitCallExpr(CallExpr *E) {
+ Decl *CalleeDecl = E->getCalleeDecl();
+ // If there is no callee, it might be an overload set or inside a template
+ if (auto *Callee = dyn_cast_or_null<FunctionDecl>(CalleeDecl)) {
+ // if there are no va_args, we can forward parameters
+ if (Callee->getNumParams() == E->getNumArgs()) {
+ handleCallOrConstructExpr(Callee, E->arguments());
+ }
+ } else if (auto *Lookup = dyn_cast<UnresolvedLookupExpr>(E->getCallee())) {
+ if (auto *Callee = resolveOverload(Lookup, E)) {
+ // resolveOverload already checked NumParams == NumArgs
+ handleCallOrConstructExpr(Callee, E->arguments());
+ }
+ }
+ // continue searching only if there are unresolved parameters left.
+ return !UnresolvedParams.empty();
+ }
+
+private:
+ void handleParmVarDeclName(const FunctionDecl *Callee, size_t I) {
+ const auto *Param = Callee->getParamDecl(I);
+ // If the declaration doesn't provide a name
+ if (!Param->getDeclName().getAsIdentifierInfo()) {
+ // Try the definition
+ if (const auto *Def = Callee->getDefinition()) {
+ const auto *DefParam = Def->getParamDecl(I);
+ if (DefParam != Param &&
+ DefParam->getDeclName().getAsIdentifierInfo()) {
+ // Store the mapping declaration parameter -> definition parameter
+ ParamDeclToDef.insert({Param, DefParam});
+ }
+ }
+ }
+ }
+
+ void handleCallOrConstructExpr(FunctionDecl *Callee,
+ CallExpr::arg_range Args) {
+ size_t ArgIdx = 0;
+ bool Recurse = false;
+ for (const auto *Arg : Args) {
+ if (const auto *ArgRef = unpackArgument(Arg)) {
+ if (const auto *ArgSource = dyn_cast<ParmVarDecl>(ArgRef->getDecl())) {
+ if (UnresolvedParams.contains(ArgSource)) {
+ const auto *Param = Callee->getParamDecl(ArgIdx);
+ // If this parameter is part of an expanded pack, we need to recurse
+ if (isExpandedParameter(Param)) {
+ UnresolvedParams.insert(Param);
+ Recurse = true;
+ } else {
+ // If it is a concrete parameter, we need to check whether it
+ // provides a name, and if not check its definition
+ handleParmVarDeclName(Callee, ArgIdx);
+ }
+ // This parameter has now been resolved
+ UnresolvedParams.erase(ArgSource);
+ ForwardedParams.insert({ArgSource, Param});
+ }
+ }
+ }
+ ArgIdx++;
+ }
+ if (Recurse && !TraversedFunctions.contains(Callee)) {
+ TraversalQueue.push_back(Callee);
+ }
+ }
+
+ FunctionDecl *resolveOverload(UnresolvedLookupExpr *Lookup, CallExpr *D) {
+ FunctionDecl *MatchingDecl = nullptr;
+ if (!Lookup->requiresADL()) {
+ // Check whether there is a single overload with this number of parameters
+ for (auto *Candidate : Lookup->decls()) {
+ if (auto *FuncCandidate = dyn_cast_or_null<FunctionDecl>(Candidate)) {
+ if (FuncCandidate->getNumParams() == D->getNumArgs()) {
+ if (MatchingDecl) {
+ // there are multiple candidates - abort
+ return nullptr;
+ }
+ MatchingDecl = FuncCandidate;
+ }
+ }
+ }
+ }
+ return MatchingDecl;
+ }
+
+ const Expr *unpackImplicitCast(const Expr *E) {
+ if (const auto *Cast = dyn_cast<ImplicitCastExpr>(E)) {
+ return Cast->getSubExpr();
+ }
+ return E;
+ }
+
+ const Expr *unpackForward(const Expr *E) {
+ if (const auto *Call = dyn_cast<CallExpr>(E)) {
+ const auto Callee = Call->getBuiltinCallee();
+ if (Callee == Builtin::BIforward) {
+ return Call->getArg(0);
+ }
+ }
+ return E;
+ }
+
+ const DeclRefExpr *unpackArgument(const Expr *E) {
+ E = unpackImplicitCast(E);
+ E = unpackForward(E);
+ return dyn_cast<DeclRefExpr>(E);
+ }
+
+ const ParmVarDecl *getForwardedParameter(const ParmVarDecl *Param) {
+ auto It = ForwardedParams.find(Param);
+ if (It == ForwardedParams.end()) {
+ return Param;
+ }
+ // path compression
+ It->second = getForwardedParameter(It->second);
+ return It->second;
+ }
+
+ const ParmVarDecl *getNamedParameter(const ParmVarDecl *Param) {
+ auto It = ParamDeclToDef.find(Param);
+ if (It == ParamDeclToDef.end()) {
+ return Param;
+ }
+ return It->second;
+ }
+
+ llvm::DenseSet<const ParmVarDecl *> UnresolvedParams;
+ std::deque<FunctionDecl *> TraversalQueue;
+ llvm::DenseSet<const FunctionDecl *> TraversedFunctions;
+ llvm::DenseMap<const ParmVarDecl *, const ParmVarDecl *> ForwardedParams;
+ // For parameters that don't have a name in the function declaration, but do
+ // have one in the definition, store this mapping here.
+ llvm::DenseMap<const ParmVarDecl *, const ParmVarDecl *> ParamDeclToDef;
+};
+
class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
public:
InlayHintVisitor(std::vector<InlayHint> &Results, ParsedAST &AST,
@@ -392,21 +581,29 @@
if (Ctor->isCopyOrMoveConstructor())
return;
- // Don't show hints for variadic parameters.
- size_t FixedParamCount = getFixedParamCount(Callee);
- size_t ArgCount = std::min(FixedParamCount, Args.size());
- auto Params = Callee->parameters();
+ SmallVector<const ParmVarDecl *> Params{Callee->param_begin(),
+ Callee->param_end()};
- NameVec ParameterNames = chooseParameterNames(Callee, ArgCount);
+ FwdVisitor.resolveForwardedParameters(Callee, Params);
+
+ NameVec ParameterNames = chooseParameterNames(Params);
// Exclude setters (i.e. functions with one argument whose name begins with
// "set"), as their parameter name is also not likely to be interesting.
if (isSetter(Callee, ParameterNames))
return;
- for (size_t I = 0; I < ArgCount; ++I) {
+ // Exclude builtins like std::move and std::forward
+ if (Callee->getBuiltinID() == Builtin::BImove ||
+ Callee->getBuiltinID() == Builtin::BIforward)
+ return;
+
+ for (size_t I = 0; I < Params.size(); ++I) {
StringRef Name = ParameterNames[I];
- bool NameHint = shouldHintName(Args[I], Name);
+ // Skip unexpanded pack expansion expressions
+ if (I >= Args.size() || isa<PackExpansionExpr>(Args[I]))
+ break;
+ bool NameHint = shouldHintName(Args[I], Params[I], Name);
bool ReferenceHint = shouldHintReference(Params[I]);
if (NameHint || ReferenceHint) {
@@ -440,7 +637,8 @@
return WhatItIsSetting.equals_insensitive(ParamNames[0]);
}
- bool shouldHintName(const Expr *Arg, StringRef ParamName) {
+ bool shouldHintName(const Expr *Arg, const ParmVarDecl *Param,
+ StringRef ParamName) {
if (ParamName.empty())
return false;
@@ -453,6 +651,11 @@
if (isPrecededByParamNameComment(Arg, ParamName))
return false;
+ // Exclude parameters that are unresolved parameter packs
+ if (isExpandedParameter(Param)) {
+ return false;
+ }
+
return true;
}
@@ -507,23 +710,12 @@
return {};
}
- NameVec chooseParameterNames(const FunctionDecl *Callee, size_t ArgCount) {
- // The current strategy here is to use all the parameter names from the
- // canonical declaration, unless they're all empty, in which case we
- // use all the parameter names from the definition (in present in the
- // translation unit).
- // We could try a bit harder, e.g.:
- // - try all re-declarations, not just canonical + definition
- // - fall back arg-by-arg rather than wholesale
-
- NameVec ParameterNames = getParameterNamesForDecl(Callee, ArgCount);
-
- if (llvm::all_of(ParameterNames, std::mem_fn(&StringRef::empty))) {
- if (const FunctionDecl *Def = Callee->getDefinition()) {
- ParameterNames = getParameterNamesForDecl(Def, ArgCount);
- }
+ NameVec
+ chooseParameterNames(const SmallVector<const ParmVarDecl *> Parameters) {
+ NameVec ParameterNames(Parameters.size());
+ for (size_t I = 0; I < Parameters.size(); I++) {
+ ParameterNames[I] = getSimpleName(*Parameters[I]);
}
- assert(ParameterNames.size() == ArgCount);
// Standard library functions often have parameter names that start
// with underscores, which makes the hints noisy, so strip them out.
@@ -537,26 +729,6 @@
Name = Name.ltrim('_');
}
- // Return the number of fixed parameters Function has, that is, not counting
- // parameters that are variadic (instantiated from a parameter pack) or
- // C-style varargs.
- static size_t getFixedParamCount(const FunctionDecl *Function) {
- if (FunctionTemplateDecl *Template = Function->getPrimaryTemplate()) {
- FunctionDecl *F = Template->getTemplatedDecl();
- size_t Result = 0;
- for (ParmVarDecl *Parm : F->parameters()) {
- if (Parm->isParameterPack()) {
- break;
- }
- ++Result;
- }
- return Result;
- }
- // C-style varargs don't need special handling, they're already
- // not included in getNumParams().
- return Function->getNumParams();
- }
-
static StringRef getSimpleName(const NamedDecl &D) {
if (IdentifierInfo *Ident = D.getDeclName().getAsIdentifierInfo()) {
return Ident->getName();
@@ -565,17 +737,6 @@
return StringRef();
}
- NameVec getParameterNamesForDecl(const FunctionDecl *Function,
- size_t ArgCount) {
- NameVec Result;
- for (size_t I = 0; I < ArgCount; ++I) {
- const ParmVarDecl *Parm = Function->getParamDecl(I);
- assert(Parm);
- Result.emplace_back(getSimpleName(*Parm));
- }
- return Result;
- }
-
// We pass HintSide rather than SourceLocation because we want to ensure
// it is in the same file as the common file range.
void addInlayHint(SourceRange R, HintSide Side, InlayHintKind Kind,
@@ -645,6 +806,7 @@
FileID MainFileID;
StringRef MainFileBuf;
const HeuristicResolver *Resolver;
+ ForwardingParameterVisitor FwdVisitor;
// We want to suppress default template arguments, but otherwise print
// canonical types. Unfortunately, they're conflicting policies so we can't
// have both. For regular types, suppressing template arguments is more
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
