upsj created this revision.
upsj added a reviewer: nridge.
Herald added subscribers: usaxena95, kadircet, arphaman.
Herald added a project: All.
upsj updated this revision to Diff 426127.
upsj added a comment.
upsj added a reviewer: sammccall.
upsj updated this revision to Diff 426224.
upsj updated this revision to Diff 426225.
upsj published this revision for review.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
remove accidentally committed file
upsj added a comment.
remove dependency on D124359 <https://reviews.llvm.org/D124359> and add tests
upsj added a comment.
remove unnecessary changes
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:388
+ // arguments
+ auto ForwardedParmMatcher = compoundStmt(forEachDescendant(
+ invocation(
----------------
We tend not to use ASTMatchers for these kind of tasks in clangd (except when
embedding clang-tidy checks of course).
I guess the biggest technical reason is it's hard to reason about their
performance so they end up slow and opaque (and indeed the clang-tidy checks
are slow and we haven't got a clear idea how to fix it). It's also easy to
match too much.
But part of it is just convention - because we have more RecursiveASTVisitors,
the maintainers are more familiar with the quirks there than the quirks of
matchers.
---
To be clear, I don't see any specific problems with this code! But we still
might ask to change it as there are costs to mixing styles, and we don't want
to end up in a situation where a bug fix requires choosing between an expensive
hasAncestor() matcher and rewriting the logic.
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:388
+ // arguments
+ auto ForwardedParmMatcher = compoundStmt(forEachDescendant(
+ invocation(
----------------
sammccall wrote:
> We tend not to use ASTMatchers for these kind of tasks in clangd (except when
> embedding clang-tidy checks of course).
>
> I guess the biggest technical reason is it's hard to reason about their
> performance so they end up slow and opaque (and indeed the clang-tidy checks
> are slow and we haven't got a clear idea how to fix it). It's also easy to
> match too much.
>
> But part of it is just convention - because we have more
> RecursiveASTVisitors, the maintainers are more familiar with the quirks there
> than the quirks of matchers.
>
> ---
>
> To be clear, I don't see any specific problems with this code! But we still
> might ask to change it as there are costs to mixing styles, and we don't want
> to end up in a situation where a bug fix requires choosing between an
> expensive hasAncestor() matcher and rewriting the logic.
That makes sense. From the Visitor standpoint, do I understand correctly that
you mean something like remembering the top-level templated `FunctionDecl` (or
stack of `FunctionDecl`s if we have something like nested Lambdas?) and check
every `CallExpr` and `CXXConstructExpr` for `ParmVarDecls` or
`std::forward(ParmVarDecl)` matching the parameters of the higher-level
`FunctionDecls`? That means basically lazily evaluating the parameter names, so
more storage inside the Visitor, but allows us to do recursive resolution in a
rather straightforward fashion.
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:255
+ )cpp",
+ ExpectedHint{"a: ", "param"});
+}
----------------
I haven't been able to figure out why, but for some reason the
`CXXConstructExpr` and `CXXTemporaryObjectExpr` are not being picked up by the
`RecursiveASTVisitor`, while in a similar situation below, the `CallExpr` gets
picked up by `VisitCallExpr`.
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:270
+ )cpp",
+ ExpectedHint{"a: ", "wrapped"},
+ ExpectedHint{"a: ", "param"});
----------------
This is a bit of an unfortunate side-effect of looking at instantiated
templates.
This adds special-case treatment for parameter packs in make_unique-like
functions to forward parameter names to inlay hints.
Repository:
rG LLVM Github Monorepo
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
@@ -162,6 +162,159 @@
ExpectedHint{"good: ", "good"});
}
+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 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, VariadicForwardedConstructor) {
+ // No hint for anonymous variadic parameter
+ // The 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{$wrapped[[std::forward<Args>(args)...]]}; }
+ void baz() {
+ bar<S>($param[[42]]);
+ }
+ )cpp",
+ ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+ // No hint for anonymous variadic parameter
+ assertParameterHints(R"cpp(
+ struct S { S(int a); };
+ template <typename T, typename... Args>
+ T bar(Args&&... args) { return T{$wrapped[[args...]]}; }
+ void baz() {
+ bar<S>($param[[42]]);
+ }
+ )cpp",
+ ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwardedNewConstructor) {
+ // No hint for anonymous variadic parameter
+ // The 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{$wrapped[[std::forward<Args>(args)...]]}; }
+ void baz() {
+ bar<S>($param[[42]]);
+ }
+ )cpp",
+ ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+ // No hint for anonymous variadic parameter
+ assertParameterHints(R"cpp(
+ struct S { S(int a); };
+ template <typename T, typename... Args>
+ T* bar(Args&&... args) { return new T{$wrapped[[args...]]}; }
+ void baz() {
+ bar<S>($param[[42]]);
+ }
+ )cpp",
+ ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwarded) {
+ // No hint for anonymous variadic parameter
+ // The 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($wrapped[[std::forward<Args>(args)...]]); }
+ void baz() {
+ bar($param[[42]]);
+ }
+ )cpp",
+ ExpectedHint{"a: ", "wrapped"},
+ ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlain) {
+ // No hint for anonymous variadic parameter
+ assertParameterHints(R"cpp(
+ void foo(int a);
+ template <typename... Args>
+ void bar(Args&&... args) { return foo($wrapped[[args...]]); }
+ void baz() {
+ bar($param[[42]]);
+ }
+ )cpp",
+ ExpectedHint{"a: ", "wrapped"},
+ ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, MatchingNameVariadicForwarded) {
+ // No hint for anonymous variadic parameter
+ // The 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($wrapped[[std::forward<Args>(args)...]]); }
+ void baz() {
+ int a;
+ bar(a);
+ }
+ )cpp",
+ ExpectedHint{"a: ", "wrapped"});
+}
+
+TEST(ParameterHints, MatchingNameVariadicPlain) {
+ // No hint for anonymous variadic parameter
+ assertParameterHints(R"cpp(
+ void foo(int a);
+ template <typename... Args>
+ void bar(Args&&... args) { return foo($wrapped[[args...]]); }
+ void baz() {
+ int a;
+ bar(a);
+ }
+ )cpp",
+ ExpectedHint{"a: ", "wrapped"});
+}
+
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
@@ -11,8 +11,12 @@
#include "HeuristicResolver.h"
#include "ParsedAST.h"
#include "clang/AST/DeclarationName.h"
+#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/Builtins.h"
#include "clang/Basic/SourceManager.h"
#include "llvm/ADT/ScopeExit.h"
@@ -20,6 +24,10 @@
namespace clangd {
namespace {
+AST_MATCHER(CallExpr, isStdForward) {
+ return Node.getBuiltinCallee() == Builtin::BIforward;
+}
+
// For now, inlay hints are always anchored at the left or right of their range.
enum class HintSide { Left, Right };
@@ -369,6 +377,66 @@
private:
using NameVec = SmallVector<StringRef, 8>;
+ llvm::SmallVector<const ParmVarDecl *> matchForwardedParams(
+ const FunctionDecl *Callee,
+ const llvm::ArrayRef<const ParmVarDecl *> &VariadicParams) {
+ // Store forwarded ParmVarDecl if found, nullptr otherwise
+ llvm::SmallVector<const ParmVarDecl *> Result(VariadicParams.size());
+ if (!Callee->hasBody()) {
+ return Result;
+ }
+ using namespace clang::ast_matchers;
+ // Match either std::forward<Args>(args)... or args... inside invocation
+ // arguments
+ auto ForwardedParmMatcher = compoundStmt(forEachDescendant(
+ invocation(
+ forEach(implicitCastExpr(
+ anyOf(has(declRefExpr(
+ hasDeclaration(parmVarDecl().bind("arg")))),
+ has(callExpr(isStdForward(),
+ has(declRefExpr(hasDeclaration(
+ parmVarDecl().bind("arg"))))))))
+ .bind("cast")))
+ .bind("call")));
+ auto Matches = match(ForwardedParmMatcher, *Callee->getBody(), AST);
+ // Store lookup table to find location of ParmVarDecls in VariadicParams
+ llvm::SmallDenseMap<const ParmVarDecl *, size_t> ParamLocations;
+ size_t ParamIdx = 0;
+ for (const auto *Param : VariadicParams) {
+ ParamLocations.insert({Param, ParamIdx});
+ ParamIdx++;
+ }
+ for (auto Entry : Matches) {
+ const auto *Param = Entry.getNodeAs<ParmVarDecl>("arg");
+ const auto LocIt = ParamLocations.find(Param);
+ // If the argument in the forwarded call matches one of our VariadicParams
+ if (LocIt != ParamLocations.end()) {
+ const auto *Cast = Entry.getNodeAs<ImplicitCastExpr>("cast");
+ const auto *Call = Entry.getNodeAs<CallExpr>("call");
+ const auto *Constructor = Entry.getNodeAs<CXXConstructExpr>("call");
+ // Grab the corresponding forwarded parameter
+ if (Call) {
+ auto CallLoc = std::distance(
+ Call->arg_begin(),
+ std::find(Call->arg_begin(), Call->arg_end(), Cast));
+ assert(CallLoc < Call->getNumArgs());
+ Result[LocIt->second] =
+ Call->getCalleeDecl()->getAsFunction()->getParamDecl(CallLoc);
+ } else {
+ assert(Constructor);
+ auto CallLoc = std::distance(Constructor->arg_begin(),
+ std::find(Constructor->arg_begin(),
+ Constructor->arg_end(), Cast));
+ assert(CallLoc < Constructor->getNumArgs());
+ Result[LocIt->second] =
+ Constructor->getConstructor()->getParamDecl(CallLoc);
+ }
+ }
+ }
+
+ return Result;
+ }
+
// The purpose of Anchor is to deal with macros. It should be the call's
// opening or closing parenthesis or brace. (Always using the opening would
// make more sense but CallExpr only exposes the closing.) We heuristically
@@ -389,9 +457,10 @@
if (Ctor->isCopyOrMoveConstructor())
return;
- // Don't show hints for variadic parameters.
+ // Skip variadic parameters for now.
size_t FixedParamCount = getFixedParamCount(Callee);
size_t ArgCount = std::min(FixedParamCount, Args.size());
+ const auto Params = Callee->parameters();
NameVec ParameterNames = chooseParameterNames(Callee, ArgCount);
@@ -400,7 +469,18 @@
if (isSetter(Callee, ParameterNames))
return;
- for (size_t I = 0; I < ArgCount; ++I) {
+ // If the function call contains a variadic parameter, attempt to forward
+ // the parameter names from the wrapped function
+ if (Args.size() > FixedParamCount && Args.size() == Params.size()) {
+ auto ForwardedParams = matchForwardedParams(
+ Callee, Params.take_back(Args.size() - ArgCount));
+ for (size_t I = ArgCount; I < Args.size(); ++I) {
+ const auto *Parm = ForwardedParams[I - ArgCount];
+ ParameterNames.emplace_back(Parm ? getSimpleName(*Parm) : "");
+ }
+ }
+
+ for (size_t I = 0; I < ParameterNames.size(); ++I) {
StringRef Name = ParameterNames[I];
if (!shouldHint(Args[I], Name))
continue;
@@ -497,7 +577,7 @@
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
+ // use all the parameter names from the definition (if present in the
// translation unit).
// We could try a bit harder, e.g.:
// - try all re-declarations, not just canonical + definition
@@ -563,7 +643,7 @@
return Result;
}
- // We pass HintSide rather than SourceLocation because we want to ensure
+ // 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,
llvm::StringRef Prefix, llvm::StringRef Label,
@@ -617,7 +697,8 @@
std::string TypeName = T.getAsString(Policy);
if (TypeName.length() < TypeNameLimit)
addInlayHint(R, HintSide::Right, InlayHintKind::TypeHint, Prefix,
- TypeName, /*Suffix=*/"");
+ TypeName,
+ /*Suffix=*/"");
}
void addDesignatorHint(SourceRange R, llvm::StringRef Text) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
