[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa638648fef76: [clangd] add inlay hints for std::forward-ed 
parameter packs (authored by upsj).

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/AST.cpp
  clang-tools-extra/clangd/AST.h
  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
@@ -174,6 +174,43 @@
   )cpp");
 }
 
+TEST(ParameterHints, NoNameVariadicDeclaration) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+template 
+void foo(Args&& ...);
+void bar() {
+  foo(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicForwarded) {
+  // No hint for anonymous variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward(args)...); }
+void baz() {
+  bar(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+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(
@@ -186,6 +223,36 @@
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, NameInDefinitionVariadic) {
+  // Parameter name picked up from definition in a resolved forwarded parameter.
+  assertParameterHints(R"cpp(
+void foo(int, int);
+template 
+void bar(Args... args) {
+  foo(args...);
+}
+void baz() {
+  bar($param1[[42]], $param2[[42]]);
+}
+void foo(int a, int b) {};
+  )cpp",
+   ExpectedHint{"a: ", "param1"},
+   ExpectedHint{"b: ", "param2"});
+}
+
 TEST(ParameterHints, NameMismatch) {
   // Prefer name from declaration.
   assertParameterHints(R"cpp(
@@ -258,6 +325,455 @@
ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, VariadicForwardedConstructor) {
+  // Name hint for variadic parameter using std::forward in a constructor call
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter in a constructor call
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwardedNewConstructor) {
+  // Name hint for variadic parameter using std::forward in a new expression
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter in a new expression
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwarded) {
+  // Name for variadic parameter using std::forward
+  // This prototype of std::forward is sufficient for clang to recognize it
+  

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added a comment.

@nridge @sammccall if you don't have any further comments, I will commit this 
tomorrow, and maybe think about how to extend it to signature help/code 
completion :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 441646.
upsj added a comment.

don't add reference hints to unresolved parameter packs


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/AST.cpp
  clang-tools-extra/clangd/AST.h
  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
@@ -174,6 +174,43 @@
   )cpp");
 }
 
+TEST(ParameterHints, NoNameVariadicDeclaration) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+template 
+void foo(Args&& ...);
+void bar() {
+  foo(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicForwarded) {
+  // No hint for anonymous variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward(args)...); }
+void baz() {
+  bar(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+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(
@@ -186,6 +223,36 @@
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, NameInDefinitionVariadic) {
+  // Parameter name picked up from definition in a resolved forwarded parameter.
+  assertParameterHints(R"cpp(
+void foo(int, int);
+template 
+void bar(Args... args) {
+  foo(args...);
+}
+void baz() {
+  bar($param1[[42]], $param2[[42]]);
+}
+void foo(int a, int b) {};
+  )cpp",
+   ExpectedHint{"a: ", "param1"},
+   ExpectedHint{"b: ", "param2"});
+}
+
 TEST(ParameterHints, NameMismatch) {
   // Prefer name from declaration.
   assertParameterHints(R"cpp(
@@ -258,6 +325,455 @@
ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, VariadicForwardedConstructor) {
+  // Name hint for variadic parameter using std::forward in a constructor call
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter in a constructor call
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwardedNewConstructor) {
+  // Name hint for variadic parameter using std::forward in a new expression
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter in a new expression
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwarded) {
+  // Name for variadic parameter using std::forward
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+  

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:483
+   !Type.getNonReferenceType().isConstQualified() &&
+   !isExpandedParameterPack(Param);
   }

sammccall wrote:
> upsj wrote:
> > sammccall wrote:
> > > upsj wrote:
> > > > sammccall wrote:
> > > > > sammccall wrote:
> > > > > > nridge wrote:
> > > > > > > sammccall wrote:
> > > > > > > > why is this check needed if we already decline to provide a 
> > > > > > > > name for the parameter on line 534 in chooseParameterNames?
> > > > > > > `shouldHintName` and `shouldHintReference` are [two independent 
> > > > > > > conditions](https://searchfox.org/llvm/rev/508eb41d82ca956c30950d9a16b522a29aeeb333/clang-tools-extra/clangd/InlayHints.cpp#411-418)
> > > > > > >  governing whether we show the parameter name and/or a `&` 
> > > > > > > indicating pass-by-mutable-ref, respectively
> > > > > > > 
> > > > > > > (I did approve the [patch](https://reviews.llvm.org/D124359) that 
> > > > > > > introduced `shouldHintReference` myself, hope that's ok)
> > > > > > Thanks, that makes sense! I just hadn't understood that change.
> > > > > What exactly *is* the motivation for suppressing reference hints in 
> > > > > the pack case?
> > > > > 
> > > > > (I can imagine there are cases where they're annoying, but it's hard 
> > > > > to know if the condition is right without knowing what those are)
> > > > I added an explanation. Basically, if we are unable to figure out which 
> > > > parameter the arguments are being forwarded to, the type of the 
> > > > ParmVarDecl for `Args&&...` gets deduced as `T&` or `T&&`, so that 
> > > > would mean even though we don't know whether the argument will 
> > > > eventually be forwarded to a reference parameter, we still claim all 
> > > > mutable lvalue arguments will be mutated, which IMO introduces more 
> > > > noise than necessary. But I think there are also good arguments for 
> > > > adding them to be safe.
> > > > 
> > > > There is another detail here, which is that we don't record whether we 
> > > > used std::forward, so the corresponding rvalue-to-lvalue conversions 
> > > > may lead to some unnecessary & annotations for rvalue arguments.
> > > This makes sense, the comment explains well, thank you!
> > > I have a couple of quibbles, up to you whether to change the logic.
> > > 
> > > #1: There's an unstated assumption that pack arguments *will* be 
> > > forwarded (there are other things we can do with them, like use them in 
> > > fold-expressions). It's a pretty good assumption but if the comment talks 
> > > about forwarding, it should probably mention explicitly ("it's likely the 
> > > params will be somehow forwarded, and...")
> > > 
> > > #2: the idea is that if the reference-ness is deduced from the callsite, 
> > > then it's not meaningful as an "is the param modified" signal, it's just 
> > > "is this arg modifiable". Fair enough, but this is a property of 
> > > universal/forwarding references (T&& where T is a template param), not of 
> > > packs. So I *think* this check should rather be 
> > > !isInstantiatedFromForwardingReference(Param).
> > > But maybe that's more complexity and what you have is a good heuristic - 
> > > I think at least we should call out that it's a heuristic for the true 
> > > condition.
> > > 
> > > 
> > #1: I agree, I'll make that more clear before committing.
> > 
> > #2: Now that I think about it, there are actually two things we don't keep 
> > track of: parameters could lose their reference-ness via `Args...` instead 
> > of `Args&&...` and their rvalue-ness by not using `std::forward`. We only 
> > look at whether the innermost call takes a reference parameter, but as I 
> > said, we may lose some of that information on the way, claiming the 
> > function may modify the argument when it actually creates a copy on the way 
> > (losing reference-ness). I think the case of an rvalue being mistaken for 
> > an lvalue should not be much of an issue, since the reference annotation 
> > almost makes sense.
> > 
> > To visualize the situation: These three snippets all add &: hints to the 
> > parameter of bar
> > ```
> > void foo(int&);
> > template 
> > void bar(Args... args) { return foo(args...); }
> > void baz() {
> >   bar(1);
> > }
> > ```
> > ```
> > void foo(int&);
> > template 
> > void bar(Args&&... args) { return foo(args...); }
> > void baz() {
> >   bar(1);
> > }
> > ```
> > ```
> > void foo(int&);
> > template 
> > void bar(Args&&... args) { return foo(std::forward(args)...); }
> > void baz() {
> >   int a;
> >   bar(a);
> > }
> > ```
> > Two of these three cases probably shouldn't have this annotation?
> > parameters could lose their reference-ness via Args... instead of Args&&...
> (I'm not quite following what you mean here: if we deduce as `Args` rather 
> than `Args&&` then the parameters are not references in the first place, 
> we're passing by value)
> 
> > and their rvalue-ness by 

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 441520.
upsj marked 3 inline comments as done.
upsj added a comment.

detect whether forwarding functions preserve reference-ness and value 
categories of their arguments


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/AST.cpp
  clang-tools-extra/clangd/AST.h
  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
@@ -174,6 +174,43 @@
   )cpp");
 }
 
+TEST(ParameterHints, NoNameVariadicDeclaration) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+template 
+void foo(Args&& ...);
+void bar() {
+  foo(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicForwarded) {
+  // No hint for anonymous variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward(args)...); }
+void baz() {
+  bar(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+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(
@@ -186,6 +223,36 @@
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, NameInDefinitionVariadic) {
+  // Parameter name picked up from definition in a resolved forwarded parameter.
+  assertParameterHints(R"cpp(
+void foo(int, int);
+template 
+void bar(Args... args) {
+  foo(args...);
+}
+void baz() {
+  bar($param1[[42]], $param2[[42]]);
+}
+void foo(int a, int b) {};
+  )cpp",
+   ExpectedHint{"a: ", "param1"},
+   ExpectedHint{"b: ", "param2"});
+}
+
 TEST(ParameterHints, NameMismatch) {
   // Prefer name from declaration.
   assertParameterHints(R"cpp(
@@ -258,6 +325,444 @@
ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, VariadicForwardedConstructor) {
+  // Name hint for variadic parameter using std::forward in a constructor call
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter in a constructor call
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwardedNewConstructor) {
+  // Name hint for variadic parameter using std::forward in a new expression
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter in a new expression
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwarded) {
+  // Name for variadic parameter using std::forward
+  // This prototype of std::forward is sufficient for clang to recognize it

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:483
+   !Type.getNonReferenceType().isConstQualified() &&
+   !isExpandedParameterPack(Param);
   }

upsj wrote:
> sammccall wrote:
> > upsj wrote:
> > > sammccall wrote:
> > > > sammccall wrote:
> > > > > nridge wrote:
> > > > > > sammccall wrote:
> > > > > > > why is this check needed if we already decline to provide a name 
> > > > > > > for the parameter on line 534 in chooseParameterNames?
> > > > > > `shouldHintName` and `shouldHintReference` are [two independent 
> > > > > > conditions](https://searchfox.org/llvm/rev/508eb41d82ca956c30950d9a16b522a29aeeb333/clang-tools-extra/clangd/InlayHints.cpp#411-418)
> > > > > >  governing whether we show the parameter name and/or a `&` 
> > > > > > indicating pass-by-mutable-ref, respectively
> > > > > > 
> > > > > > (I did approve the [patch](https://reviews.llvm.org/D124359) that 
> > > > > > introduced `shouldHintReference` myself, hope that's ok)
> > > > > Thanks, that makes sense! I just hadn't understood that change.
> > > > What exactly *is* the motivation for suppressing reference hints in the 
> > > > pack case?
> > > > 
> > > > (I can imagine there are cases where they're annoying, but it's hard to 
> > > > know if the condition is right without knowing what those are)
> > > I added an explanation. Basically, if we are unable to figure out which 
> > > parameter the arguments are being forwarded to, the type of the 
> > > ParmVarDecl for `Args&&...` gets deduced as `T&` or `T&&`, so that would 
> > > mean even though we don't know whether the argument will eventually be 
> > > forwarded to a reference parameter, we still claim all mutable lvalue 
> > > arguments will be mutated, which IMO introduces more noise than 
> > > necessary. But I think there are also good arguments for adding them to 
> > > be safe.
> > > 
> > > There is another detail here, which is that we don't record whether we 
> > > used std::forward, so the corresponding rvalue-to-lvalue conversions may 
> > > lead to some unnecessary & annotations for rvalue arguments.
> > This makes sense, the comment explains well, thank you!
> > I have a couple of quibbles, up to you whether to change the logic.
> > 
> > #1: There's an unstated assumption that pack arguments *will* be forwarded 
> > (there are other things we can do with them, like use them in 
> > fold-expressions). It's a pretty good assumption but if the comment talks 
> > about forwarding, it should probably mention explicitly ("it's likely the 
> > params will be somehow forwarded, and...")
> > 
> > #2: the idea is that if the reference-ness is deduced from the callsite, 
> > then it's not meaningful as an "is the param modified" signal, it's just 
> > "is this arg modifiable". Fair enough, but this is a property of 
> > universal/forwarding references (T&& where T is a template param), not of 
> > packs. So I *think* this check should rather be 
> > !isInstantiatedFromForwardingReference(Param).
> > But maybe that's more complexity and what you have is a good heuristic - I 
> > think at least we should call out that it's a heuristic for the true 
> > condition.
> > 
> > 
> #1: I agree, I'll make that more clear before committing.
> 
> #2: Now that I think about it, there are actually two things we don't keep 
> track of: parameters could lose their reference-ness via `Args...` instead of 
> `Args&&...` and their rvalue-ness by not using `std::forward`. We only look 
> at whether the innermost call takes a reference parameter, but as I said, we 
> may lose some of that information on the way, claiming the function may 
> modify the argument when it actually creates a copy on the way (losing 
> reference-ness). I think the case of an rvalue being mistaken for an lvalue 
> should not be much of an issue, since the reference annotation almost makes 
> sense.
> 
> To visualize the situation: These three snippets all add &: hints to the 
> parameter of bar
> ```
> void foo(int&);
> template 
> void bar(Args... args) { return foo(args...); }
> void baz() {
>   bar(1);
> }
> ```
> ```
> void foo(int&);
> template 
> void bar(Args&&... args) { return foo(args...); }
> void baz() {
>   bar(1);
> }
> ```
> ```
> void foo(int&);
> template 
> void bar(Args&&... args) { return foo(std::forward(args)...); }
> void baz() {
>   int a;
>   bar(a);
> }
> ```
> Two of these three cases probably shouldn't have this annotation?
> parameters could lose their reference-ness via Args... instead of Args&&...
(I'm not quite following what you mean here: if we deduce as `Args` rather than 
`Args&&` then the parameters are not references in the first place, we're 
passing by value)

> and their rvalue-ness by not using std::forward
Yes. Fundamentally if we're deducing the ref type then we should be looking for 
a concrete signal of how the value is ultimately used, which involves tracking 
casts like std::forward. 

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj marked an inline comment as done.
upsj added a comment.

yes, I have commit access




Comment at: clang-tools-extra/clangd/InlayHints.cpp:483
+   !Type.getNonReferenceType().isConstQualified() &&
+   !isExpandedParameterPack(Param);
   }

sammccall wrote:
> upsj wrote:
> > sammccall wrote:
> > > sammccall wrote:
> > > > nridge wrote:
> > > > > sammccall wrote:
> > > > > > why is this check needed if we already decline to provide a name 
> > > > > > for the parameter on line 534 in chooseParameterNames?
> > > > > `shouldHintName` and `shouldHintReference` are [two independent 
> > > > > conditions](https://searchfox.org/llvm/rev/508eb41d82ca956c30950d9a16b522a29aeeb333/clang-tools-extra/clangd/InlayHints.cpp#411-418)
> > > > >  governing whether we show the parameter name and/or a `&` indicating 
> > > > > pass-by-mutable-ref, respectively
> > > > > 
> > > > > (I did approve the [patch](https://reviews.llvm.org/D124359) that 
> > > > > introduced `shouldHintReference` myself, hope that's ok)
> > > > Thanks, that makes sense! I just hadn't understood that change.
> > > What exactly *is* the motivation for suppressing reference hints in the 
> > > pack case?
> > > 
> > > (I can imagine there are cases where they're annoying, but it's hard to 
> > > know if the condition is right without knowing what those are)
> > I added an explanation. Basically, if we are unable to figure out which 
> > parameter the arguments are being forwarded to, the type of the ParmVarDecl 
> > for `Args&&...` gets deduced as `T&` or `T&&`, so that would mean even 
> > though we don't know whether the argument will eventually be forwarded to a 
> > reference parameter, we still claim all mutable lvalue arguments will be 
> > mutated, which IMO introduces more noise than necessary. But I think there 
> > are also good arguments for adding them to be safe.
> > 
> > There is another detail here, which is that we don't record whether we used 
> > std::forward, so the corresponding rvalue-to-lvalue conversions may lead to 
> > some unnecessary & annotations for rvalue arguments.
> This makes sense, the comment explains well, thank you!
> I have a couple of quibbles, up to you whether to change the logic.
> 
> #1: There's an unstated assumption that pack arguments *will* be forwarded 
> (there are other things we can do with them, like use them in 
> fold-expressions). It's a pretty good assumption but if the comment talks 
> about forwarding, it should probably mention explicitly ("it's likely the 
> params will be somehow forwarded, and...")
> 
> #2: the idea is that if the reference-ness is deduced from the callsite, then 
> it's not meaningful as an "is the param modified" signal, it's just "is this 
> arg modifiable". Fair enough, but this is a property of universal/forwarding 
> references (T&& where T is a template param), not of packs. So I *think* this 
> check should rather be !isInstantiatedFromForwardingReference(Param).
> But maybe that's more complexity and what you have is a good heuristic - I 
> think at least we should call out that it's a heuristic for the true 
> condition.
> 
> 
#1: I agree, I'll make that more clear before committing.

#2: Now that I think about it, there are actually two things we don't keep 
track of: parameters could lose their reference-ness via `Args...` instead of 
`Args&&...` and their rvalue-ness by not using `std::forward`. We only look at 
whether the innermost call takes a reference parameter, but as I said, we may 
lose some of that information on the way, claiming the function may modify the 
argument when it actually creates a copy on the way (losing reference-ness). I 
think the case of an rvalue being mistaken for an lvalue should not be much of 
an issue, since the reference annotation almost makes sense.

To visualize the situation: These three snippets all add &: hints to the 
parameter of bar
```
void foo(int&);
template 
void bar(Args... args) { return foo(args...); }
void baz() {
  bar(1);
}
```
```
void foo(int&);
template 
void bar(Args&&... args) { return foo(args...); }
void baz() {
  bar(1);
}
```
```
void foo(int&);
template 
void bar(Args&&... args) { return foo(std::forward(args)...); }
void baz() {
  int a;
  bar(a);
}
```
Two of these three cases probably shouldn't have this annotation?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Thanks for the readability improvements! I've forgotten if you have commit 
access?

This stuff is complicated and I'm definitely going to forget the reasoning, the 
intuitive explanations are going to help a lot if changes are needed in future.
(Maybe it's just me, but the formal language around instantiations, deductions, 
etc makes my eyes glaze over - as if it's the words themselves rather than how 
you use them!)




Comment at: clang-tools-extra/clangd/AST.cpp:690
+getPackTemplateParameter(const FunctionDecl *Callee) {
+  if (const auto *TemplateDecl = Callee->getPrimaryTemplate()) {
+auto TemplateParams = TemplateDecl->getTemplateParameters()->asArray();

upsj wrote:
> sammccall wrote:
> > This is doing something pretty strange if Callee is a function template 
> > specialization.
> > 
> > It's not clear to me whether this function should be handling that case 
> > (which AFAICS it doesn't, but could inspect the specialization kind), or 
> > whether resolveForwardingParameters is responsible for not calling this 
> > function in that case (in which case we should probably have an assert 
> > here).
> > 
> > Can you also add a test case that function template specialization doesn't 
> > confuse us? i.e. it should return the parmvardecls from the 
> > specialization's definition.
> Do the new tests `VariadicNameFromSpecialization(Recursive)?` match what you 
> had in mind?
Yes, that's fantastic, thank you!



Comment at: clang-tools-extra/clangd/InlayHints.cpp:483
+   !Type.getNonReferenceType().isConstQualified() &&
+   !isExpandedParameterPack(Param);
   }

upsj wrote:
> sammccall wrote:
> > sammccall wrote:
> > > nridge wrote:
> > > > sammccall wrote:
> > > > > why is this check needed if we already decline to provide a name for 
> > > > > the parameter on line 534 in chooseParameterNames?
> > > > `shouldHintName` and `shouldHintReference` are [two independent 
> > > > conditions](https://searchfox.org/llvm/rev/508eb41d82ca956c30950d9a16b522a29aeeb333/clang-tools-extra/clangd/InlayHints.cpp#411-418)
> > > >  governing whether we show the parameter name and/or a `&` indicating 
> > > > pass-by-mutable-ref, respectively
> > > > 
> > > > (I did approve the [patch](https://reviews.llvm.org/D124359) that 
> > > > introduced `shouldHintReference` myself, hope that's ok)
> > > Thanks, that makes sense! I just hadn't understood that change.
> > What exactly *is* the motivation for suppressing reference hints in the 
> > pack case?
> > 
> > (I can imagine there are cases where they're annoying, but it's hard to 
> > know if the condition is right without knowing what those are)
> I added an explanation. Basically, if we are unable to figure out which 
> parameter the arguments are being forwarded to, the type of the ParmVarDecl 
> for `Args&&...` gets deduced as `T&` or `T&&`, so that would mean even though 
> we don't know whether the argument will eventually be forwarded to a 
> reference parameter, we still claim all mutable lvalue arguments will be 
> mutated, which IMO introduces more noise than necessary. But I think there 
> are also good arguments for adding them to be safe.
> 
> There is another detail here, which is that we don't record whether we used 
> std::forward, so the corresponding rvalue-to-lvalue conversions may lead to 
> some unnecessary & annotations for rvalue arguments.
This makes sense, the comment explains well, thank you!
I have a couple of quibbles, up to you whether to change the logic.

#1: There's an unstated assumption that pack arguments *will* be forwarded 
(there are other things we can do with them, like use them in 
fold-expressions). It's a pretty good assumption but if the comment talks about 
forwarding, it should probably mention explicitly ("it's likely the params will 
be somehow forwarded, and...")

#2: the idea is that if the reference-ness is deduced from the callsite, then 
it's not meaningful as an "is the param modified" signal, it's just "is this 
arg modifiable". Fair enough, but this is a property of universal/forwarding 
references (T&& where T is a template param), not of packs. So I *think* this 
check should rather be !isInstantiatedFromForwardingReference(Param).
But maybe that's more complexity and what you have is a good heuristic - I 
think at least we should call out that it's a heuristic for the true condition.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:690
+getPackTemplateParameter(const FunctionDecl *Callee) {
+  if (const auto *TemplateDecl = Callee->getPrimaryTemplate()) {
+auto TemplateParams = TemplateDecl->getTemplateParameters()->asArray();

sammccall wrote:
> This is doing something pretty strange if Callee is a function template 
> specialization.
> 
> It's not clear to me whether this function should be handling that case 
> (which AFAICS it doesn't, but could inspect the specialization kind), or 
> whether resolveForwardingParameters is responsible for not calling this 
> function in that case (in which case we should probably have an assert here).
> 
> Can you also add a test case that function template specialization doesn't 
> confuse us? i.e. it should return the parmvardecls from the specialization's 
> definition.
Do the new tests `VariadicNameFromSpecialization(Recursive)?` match what you 
had in mind?



Comment at: clang-tools-extra/clangd/InlayHints.cpp:483
+   !Type.getNonReferenceType().isConstQualified() &&
+   !isExpandedParameterPack(Param);
   }

sammccall wrote:
> sammccall wrote:
> > nridge wrote:
> > > sammccall wrote:
> > > > why is this check needed if we already decline to provide a name for 
> > > > the parameter on line 534 in chooseParameterNames?
> > > `shouldHintName` and `shouldHintReference` are [two independent 
> > > conditions](https://searchfox.org/llvm/rev/508eb41d82ca956c30950d9a16b522a29aeeb333/clang-tools-extra/clangd/InlayHints.cpp#411-418)
> > >  governing whether we show the parameter name and/or a `&` indicating 
> > > pass-by-mutable-ref, respectively
> > > 
> > > (I did approve the [patch](https://reviews.llvm.org/D124359) that 
> > > introduced `shouldHintReference` myself, hope that's ok)
> > Thanks, that makes sense! I just hadn't understood that change.
> What exactly *is* the motivation for suppressing reference hints in the pack 
> case?
> 
> (I can imagine there are cases where they're annoying, but it's hard to know 
> if the condition is right without knowing what those are)
I added an explanation. Basically, if we are unable to figure out which 
parameter the arguments are being forwarded to, the type of the ParmVarDecl for 
`Args&&...` gets deduced as `T&` or `T&&`, so that would mean even though we 
don't know whether the argument will eventually be forwarded to a reference 
parameter, we still claim all mutable lvalue arguments will be mutated, which 
IMO introduces more noise than necessary. But I think there are also good 
arguments for adding them to be safe.

There is another detail here, which is that we don't record whether we used 
std::forward, so the corresponding rvalue-to-lvalue conversions may lead to 
some unnecessary & annotations for rvalue arguments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 440921.
upsj marked 14 inline comments as done.
upsj added a comment.

- simplify parameter pack detection
- improve function naming
- make handling of unexpanded packs and varargs more visible
- add tests involving template specializations
- make documentation more descriptive


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/AST.cpp
  clang-tools-extra/clangd/AST.h
  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
@@ -174,6 +174,43 @@
   )cpp");
 }
 
+TEST(ParameterHints, NoNameVariadicDeclaration) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+template 
+void foo(Args&& ...);
+void bar() {
+  foo(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicForwarded) {
+  // No hint for anonymous variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward(args)...); }
+void baz() {
+  bar(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+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(
@@ -186,6 +223,36 @@
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, NameInDefinitionVariadic) {
+  // Parameter name picked up from definition in a resolved forwarded parameter.
+  assertParameterHints(R"cpp(
+void foo(int, int);
+template 
+void bar(Args... args) {
+  foo(args...);
+}
+void baz() {
+  bar($param1[[42]], $param2[[42]]);
+}
+void foo(int a, int b) {};
+  )cpp",
+   ExpectedHint{"a: ", "param1"},
+   ExpectedHint{"b: ", "param2"});
+}
+
 TEST(ParameterHints, NameMismatch) {
   // Prefer name from declaration.
   assertParameterHints(R"cpp(
@@ -258,6 +325,389 @@
ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, VariadicForwardedConstructor) {
+  // Name hint for variadic parameter using std::forward in a constructor call
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter in a constructor call
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwardedNewConstructor) {
+  // Name hint for variadic parameter using std::forward in a new expression
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter in a new expression
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwarded) {
+  // Name for 

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:478
   bool shouldHintReference(const ParmVarDecl *Param) {
-// If the parameter is a non-const reference type, print an inlay hint
+// If the parameter is of non-const l-value reference type not originating
+// from an unresolved expanded pack, print an inlay hint

The new condition is nonobvious, but the new comment just echoes the code.

Instead add a second sentence explaining *why*.
(I can't provide a suggestion, because I don't understand why)



Comment at: clang-tools-extra/clangd/InlayHints.cpp:483
+   !Type.getNonReferenceType().isConstQualified() &&
+   !isExpandedParameterPack(Param);
   }

nridge wrote:
> sammccall wrote:
> > why is this check needed if we already decline to provide a name for the 
> > parameter on line 534 in chooseParameterNames?
> `shouldHintName` and `shouldHintReference` are [two independent 
> conditions](https://searchfox.org/llvm/rev/508eb41d82ca956c30950d9a16b522a29aeeb333/clang-tools-extra/clangd/InlayHints.cpp#411-418)
>  governing whether we show the parameter name and/or a `&` indicating 
> pass-by-mutable-ref, respectively
> 
> (I did approve the [patch](https://reviews.llvm.org/D124359) that introduced 
> `shouldHintReference` myself, hope that's ok)
Thanks, that makes sense! I just hadn't understood that change.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:483
+   !Type.getNonReferenceType().isConstQualified() &&
+   !isExpandedParameterPack(Param);
   }

sammccall wrote:
> nridge wrote:
> > sammccall wrote:
> > > why is this check needed if we already decline to provide a name for the 
> > > parameter on line 534 in chooseParameterNames?
> > `shouldHintName` and `shouldHintReference` are [two independent 
> > conditions](https://searchfox.org/llvm/rev/508eb41d82ca956c30950d9a16b522a29aeeb333/clang-tools-extra/clangd/InlayHints.cpp#411-418)
> >  governing whether we show the parameter name and/or a `&` indicating 
> > pass-by-mutable-ref, respectively
> > 
> > (I did approve the [patch](https://reviews.llvm.org/D124359) that 
> > introduced `shouldHintReference` myself, hope that's ok)
> Thanks, that makes sense! I just hadn't understood that change.
What exactly *is* the motivation for suppressing reference hints in the pack 
case?

(I can imagine there are cases where they're annoying, but it's hard to know if 
the condition is right without knowing what those are)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:483
+   !Type.getNonReferenceType().isConstQualified() &&
+   !isExpandedParameterPack(Param);
   }

sammccall wrote:
> why is this check needed if we already decline to provide a name for the 
> parameter on line 534 in chooseParameterNames?
`shouldHintName` and `shouldHintReference` are [two independent 
conditions](https://searchfox.org/llvm/rev/508eb41d82ca956c30950d9a16b522a29aeeb333/clang-tools-extra/clangd/InlayHints.cpp#411-418)
 governing whether we show the parameter name and/or a `&` indicating 
pass-by-mutable-ref, respectively

(I did approve the [patch](https://reviews.llvm.org/D124359) that introduced 
`shouldHintReference` myself, hope that's ok)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Sorry about the delay. This is complicated stuff and every time I put it down 
for a day I forget how it works!
Thanks for your hard work here.

I have some more suggestions but hopefully fairly mechanical.
Feel free to land once you're done, or send it back over to Nathan or I to 
commit.




Comment at: clang-tools-extra/clangd/AST.cpp:677
+
+// Checks if the template parameter declaration is a type parameter pack
+bool isTemplateTypeParameterPack(NamedDecl *D) {

nit: this comment doesn't really say anything that the method name doesn't say 
already.

Maybe replace with an example like "returns true for `X` in `template 
 int m;`"



Comment at: clang-tools-extra/clangd/AST.cpp:680
+  if (const auto *TTPD = dyn_cast(D)) {
+const auto *TTPT = TTPD->getTypeForDecl()->castAs();
+return TTPT->isParameterPack();

TTPD->isParameterPack() does the same thing with less code



Comment at: clang-tools-extra/clangd/AST.cpp:689
+const TemplateTypeParmType *
+getPackTemplateParameter(const FunctionDecl *Callee) {
+  if (const auto *TemplateDecl = Callee->getPrimaryTemplate()) {

it seems confusing and unneccesary to use the same name for the two versions of 
this function. The name applies to both, but they don't really do the same 
thing.

Maybe call this one getFunctionPackType and the other getUnderylingPackType?
(not get*TemplateParameter as they return a type, not the parameter decl)



Comment at: clang-tools-extra/clangd/AST.cpp:690
+getPackTemplateParameter(const FunctionDecl *Callee) {
+  if (const auto *TemplateDecl = Callee->getPrimaryTemplate()) {
+auto TemplateParams = TemplateDecl->getTemplateParameters()->asArray();

This is doing something pretty strange if Callee is a function template 
specialization.

It's not clear to me whether this function should be handling that case (which 
AFAICS it doesn't, but could inspect the specialization kind), or whether 
resolveForwardingParameters is responsible for not calling this function in 
that case (in which case we should probably have an assert here).

Can you also add a test case that function template specialization doesn't 
confuse us? i.e. it should return the parmvardecls from the specialization's 
definition.



Comment at: clang-tools-extra/clangd/AST.cpp:720
+
+class ForwardingCallVisitor
+: public RecursiveASTVisitor {

This class could use a high-level comment explaining what it does.

e.g.
```
This visitor walks over the body of an instantiated function template.
The template accepts a parameter pack and the visitor records whether
the pack parameters were forwarded to another call. For example, given:

template 
auto make_unique(Args..args) {
  return unique_ptr(new T(args...));
}

When called as `make_unique(2, 'x')` this yields a function
`make_unique` with two parameters.
The visitor records that those two parameters are forwarded to the
`constructor std::string(int, char);`.

This information is recorded in the `ForwardingInfo` (in general,
more complicated scenarios are also possible).
```



Comment at: clang-tools-extra/clangd/AST.h:212
+/// reference to one (e.g. `Args&...` or `Args&&...`).
+bool isExpandedParameterPack(const ParmVarDecl *D);
+

nit: I think isExpanded**From**ParameterPack might be clearer, up to you



Comment at: clang-tools-extra/clangd/InlayHints.cpp:398
+auto Params = resolveForwardingParameters(Callee);
+// We are only interested in expanded arguments with corresponding
+// parameters.

I can't understand what this comment is saying, can you rephrase it or provide 
an example?

From looking at the code, I'm guessing something like:
"If we're passing a parameter pack into this call, we need to give up matching 
arguments to parameters at that point as we don't know how long it is".

---

I think the interaction between getExpandedArgCount and chooseParameterNames is 
unclear and brittle here. (i.e. the invariant that 
`getExpandedArgCount(Args).size() < chooseParameterNames(Params).size()` is 
very non-local)

I'd suggest writing this more directly as:
```
for (I = 0; I < ParameterNames.size() && I < Args.size(); ++I) {
  // ... explanation ...
  if (isa(Args[I]))
break;

  // ... generate param hint ...
}
```



Comment at: clang-tools-extra/clangd/InlayHints.cpp:483
+   !Type.getNonReferenceType().isConstQualified() &&
+   !isExpandedParameterPack(Param);
   }

why is this check needed if we already decline to provide a name for the 
parameter on line 534 in chooseParameterNames?



Comment at: clang-tools-extra/clangd/InlayHints.cpp:541
+if (SimpleName.empty()) {
+  if 

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added a comment.

can you give this another look, if you have some time @sammccall?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks -- the patch looks quite good to me now!

I will defer to @sammccall for final approval in case he has any additional 
feedback.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:682
+  if (const auto *TTPD =
+  dyn_cast(TemplateParams.back())) {
+const auto *TTPT =

nridge wrote:
> upsj wrote:
> > nridge wrote:
> > > I don't think there is any requirement that a pack be a trailing 
> > > **template** parameter. For example, the following is valid:
> > > 
> > > ```
> > > template 
> > > void foo(A, B...);
> > > 
> > > void bar() {
> > >   foo(1, 2, 3);
> > > }
> > > ```
> > Do you have a suggestion for how to find this pack in general? I would like 
> > to keep this function as efficient as possible, since it's used everywhere
> > Do you have a suggestion for how to find this pack in general?
> 
> Just iterate backwards through `TemplateParams` rather than only considering 
> `TemplateParams.back()`, I suppose.
> 
> > I would like to keep this function as efficient as possible, since it's 
> > used everywhere
> 
> The `ParmVarDecl*` overload of `getPackTemplateParameter()` is called a lot 
> via the `IsExpandedPack` lambdas, but this overload is only called once per 
> depth level.
Ah thanks, of course! I got tricked by my own overloads.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 437508.
upsj marked 4 inline comments as done.
upsj added a comment.

- remove inlay hints from std::forward in tests
- identify recursive variadic calls from template, not post-processing
- allow template parameter packs to appear at any place
- improve documentation


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/AST.cpp
  clang-tools-extra/clangd/AST.h
  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
@@ -174,6 +174,43 @@
   )cpp");
 }
 
+TEST(ParameterHints, NoNameVariadicDeclaration) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+template 
+void foo(Args&& ...);
+void bar() {
+  foo(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicForwarded) {
+  // No hint for anonymous variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward(args)...); }
+void baz() {
+  bar(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+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(
@@ -186,6 +223,36 @@
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, NameInDefinitionVariadic) {
+  // Parameter name picked up from definition in a resolved forwarded parameter.
+  assertParameterHints(R"cpp(
+void foo(int, int);
+template 
+void bar(Args... args) {
+  foo(args...);
+}
+void baz() {
+  bar($param1[[42]], $param2[[42]]);
+}
+void foo(int a, int b) {};
+  )cpp",
+   ExpectedHint{"a: ", "param1"},
+   ExpectedHint{"b: ", "param2"});
+}
+
 TEST(ParameterHints, NameMismatch) {
   // Prefer name from declaration.
   assertParameterHints(R"cpp(
@@ -258,6 +325,349 @@
ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, VariadicForwardedConstructor) {
+  // Name hint for variadic parameter using std::forward in a constructor call
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter in a constructor call
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwardedNewConstructor) {
+  // Name hint for variadic parameter using std::forward in a new expression
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter in a new expression
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwarded) {
+  // Name for variadic 

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks for the update and the additional test cases!




Comment at: clang-tools-extra/clangd/AST.cpp:682
+  if (const auto *TTPD =
+  dyn_cast(TemplateParams.back())) {
+const auto *TTPT =

upsj wrote:
> nridge wrote:
> > I don't think there is any requirement that a pack be a trailing 
> > **template** parameter. For example, the following is valid:
> > 
> > ```
> > template 
> > void foo(A, B...);
> > 
> > void bar() {
> >   foo(1, 2, 3);
> > }
> > ```
> Do you have a suggestion for how to find this pack in general? I would like 
> to keep this function as efficient as possible, since it's used everywhere
> Do you have a suggestion for how to find this pack in general?

Just iterate backwards through `TemplateParams` rather than only considering 
`TemplateParams.back()`, I suppose.

> I would like to keep this function as efficient as possible, since it's used 
> everywhere

The `ParmVarDecl*` overload of `getPackTemplateParameter()` is called a lot via 
the `IsExpandedPack` lambdas, but this overload is only called once per depth 
level.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:541
 
+// Remove parameter names that occur multiple times completely.
+llvm::StringMap NameLastSeen;

upsj wrote:
> nridge wrote:
> > This is an interesting approach for handling `VariadicRecursive`.
> > 
> > I had in mind a different approach, something like keeping a 
> > `std::set SeenFunctionTemplates` in 
> > `resolveForwardingParameters()`, populating it with 
> > `CurrentFunction->getPrimaryTemplate()` on each iteration, and bailing if 
> > the same function template is seen more than once (indicating recursion). 
> > But this approach seems to work too, as a parameter name can't legitimately 
> > appear twice in a function declaration.
> > 
> > That said, maybe having such a `SeenFunctionTemplates` recursion guard 
> > would be helpful anyways, so that e.g. in `VariadicInfinite`, we would bail 
> > after a single recursion rather than going until `MaxDepth`?
> I see your point here - I would also like an AST based approach more than 
> this purely string-based one. The main issue is that if I deduplicate based 
> on the function templates, I would still be left with the first parameter 
> being named, which doesn't make much sense in something like make_tuple.
One idea is that we could return the original `Parameters` from 
`resolveFowardingParameters()` if we encounter recursion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:682
+  if (const auto *TTPD =
+  dyn_cast(TemplateParams.back())) {
+const auto *TTPT =

nridge wrote:
> I don't think there is any requirement that a pack be a trailing **template** 
> parameter. For example, the following is valid:
> 
> ```
> template 
> void foo(A, B...);
> 
> void bar() {
>   foo(1, 2, 3);
> }
> ```
Do you have a suggestion for how to find this pack in general? I would like to 
keep this function as efficient as possible, since it's used everywhere



Comment at: clang-tools-extra/clangd/InlayHints.cpp:541
 
+// Remove parameter names that occur multiple times completely.
+llvm::StringMap NameLastSeen;

nridge wrote:
> This is an interesting approach for handling `VariadicRecursive`.
> 
> I had in mind a different approach, something like keeping a 
> `std::set SeenFunctionTemplates` in 
> `resolveForwardingParameters()`, populating it with 
> `CurrentFunction->getPrimaryTemplate()` on each iteration, and bailing if the 
> same function template is seen more than once (indicating recursion). But 
> this approach seems to work too, as a parameter name can't legitimately 
> appear twice in a function declaration.
> 
> That said, maybe having such a `SeenFunctionTemplates` recursion guard would 
> be helpful anyways, so that e.g. in `VariadicInfinite`, we would bail after a 
> single recursion rather than going until `MaxDepth`?
I see your point here - I would also like an AST based approach more than this 
purely string-based one. The main issue is that if I deduplicate based on the 
function templates, I would still be left with the first parameter being named, 
which doesn't make much sense in something like make_tuple.



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:200
+  )cpp",
+   ExpectedHint{"&: ", "fwd"});
+}

nridge wrote:
> As an aside, `&` does not seem like a useful hint to show for `std::forward` 
> -- should we try to omit it? (We don't need to do it in this patch as it's 
> not really related.)
see https://reviews.llvm.org/D127859 ;)



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:467
+
+TEST(ParameterHints, VariadicVarargs) {
+  assertParameterHints(R"cpp(

nridge wrote:
> I think a variation of this test case where `foo` is also variadic, would be 
> valuable to add:
> 
> ```
> template 
> void foo(int fixed, Args&&... args);
> 
> template 
> void bar(Args&&... args) {
>   foo(args...);
> }
> 
> void baz() { 
>   bar($fixed[[41]], 42, 43);
> }
> ```
> 
> This case does seem to already work with the current patch, and I think it's 
> the more common case; I'm happy to defer the varargs one as a less important 
> edge case.
The main reason I added this is to test that the visitor doesn't break on 
varargs, the output is not that important anyways. I added your suggestion as 
well, which highlighted another issue, thanks :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 437194.
upsj marked 9 inline comments as done.
upsj added a comment.

- improve documentation
- add more edge case tests
- fix reference hints for parameter packs
- remove unnecessary headers
- fix bug in handling of tail 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/AST.cpp
  clang-tools-extra/clangd/AST.h
  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
@@ -174,6 +174,44 @@
   )cpp");
 }
 
+TEST(ParameterHints, NoNameVariadicDeclaration) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+template 
+void foo(Args&& ...);
+void bar() {
+  foo(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicForwarded) {
+  // No hint for anonymous variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward($fwd[[args]])...); }
+void baz() {
+  bar(42);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"});
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+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(
@@ -186,6 +224,36 @@
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, NameInDefinitionVariadic) {
+  // Parameter name picked up from definition in a resolved forwarded parameter.
+  assertParameterHints(R"cpp(
+void foo(int, int);
+template 
+void bar(Args... args) {
+  foo(args...);
+}
+void baz() {
+  bar($param1[[42]], $param2[[42]]);
+}
+void foo(int a, int b) {};
+  )cpp",
+   ExpectedHint{"a: ", "param1"},
+   ExpectedHint{"b: ", "param2"});
+}
+
 TEST(ParameterHints, NameMismatch) {
   // Prefer name from declaration.
   assertParameterHints(R"cpp(
@@ -258,6 +326,337 @@
ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, VariadicForwardedConstructor) {
+  // Name hint for variadic parameter using std::forward in a constructor call
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward($fwd[[args]])...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"},
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter in a constructor call
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwardedNewConstructor) {
+  // Name hint for variadic parameter using std::forward in a new expression
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward($fwd[[args]])...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"},
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter in a new expression
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Finished looking through the patch; I found it nicely organized and fairly easy 
to understand (as easy as code involving the analysis of C++ variadic templates 
can be, anyways :-D)!




Comment at: clang-tools-extra/clangd/AST.cpp:682
+  if (const auto *TTPD =
+  dyn_cast(TemplateParams.back())) {
+const auto *TTPT =

I don't think there is any requirement that a pack be a trailing **template** 
parameter. For example, the following is valid:

```
template 
void foo(A, B...);

void bar() {
  foo(1, 2, 3);
}
```



Comment at: clang-tools-extra/clangd/AST.cpp:761
+  // inspects the given callee with the given args to check whether it
+  // contains Parameters, and sets FullyResolved, PartiallyResolved and
+  // NextTarget accordingly.

These names (FullyResolved, PartiallyResolved, NextTarget) sound like they 
might be leftovers from a previous implementation?



Comment at: clang-tools-extra/clangd/AST.cpp:831
+  static FunctionDecl *resolveOverload(UnresolvedLookupExpr *Lookup,
+   CallExpr *D) {
+FunctionDecl *MatchingDecl = nullptr;

nit: `D` is an odd name for a `CallExpr`, maybe `Call` or `E`?



Comment at: clang-tools-extra/clangd/AST.cpp:888
+// Split the parameters into head, pack and tail
+auto IsExpandedPack = [&](const ParmVarDecl *P) {
+  return getPackTemplateParameter(P) == TTPT;

can just capture `TTPT` here



Comment at: clang-tools-extra/clangd/AST.cpp:899
+auto HeadIt = std::copy(Head.begin(), Head.end(), Result.begin());
+auto TailIt = std::copy(Tail.rbegin(), Tail.rbegin(), Result.rbegin());
+// Recurse on pack parameters

The second argument is presumably meant to be `Tail.rend()`



Comment at: clang-tools-extra/clangd/InlayHints.cpp:515
+  if (isExpandedParameterPack(P)) {
+ParameterNames.emplace_back();
+  } else {

let's add `// will not be hinted` for clarity



Comment at: clang-tools-extra/clangd/InlayHints.cpp:541
 
+// Remove parameter names that occur multiple times completely.
+llvm::StringMap NameLastSeen;

This is an interesting approach for handling `VariadicRecursive`.

I had in mind a different approach, something like keeping a 
`std::set SeenFunctionTemplates` in 
`resolveForwardingParameters()`, populating it with 
`CurrentFunction->getPrimaryTemplate()` on each iteration, and bailing if the 
same function template is seen more than once (indicating recursion). But this 
approach seems to work too, as a parameter name can't legitimately appear twice 
in a function declaration.

That said, maybe having such a `SeenFunctionTemplates` recursion guard would be 
helpful anyways, so that e.g. in `VariadicInfinite`, we would bail after a 
single recursion rather than going until `MaxDepth`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Will continue looking at the implementation tomorrow, for now a few minor 
comments and a suggested variation on a testcase.




Comment at: clang-tools-extra/clangd/AST.h:19
 #include "clang/AST/DeclObjC.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/NestedNameSpecifier.h"

nit: this include seems unnecessary given the code being added



Comment at: clang-tools-extra/clangd/AST.h:210
+
+/// Checks whether D is the expanded form of a parameter pack
+bool isExpandedParameterPack(const ParmVarDecl *D);

This function does something a bit more specific than what this description 
suggests: given a type parameter pack `typename... T`, it will return true for 
function parameters instantiated from a parameter pack of type `T...` or 
`T&...` or `T&&...`, but not `T*...` or `vector...` or anything else.

Suggested description:

```
/// Checks whether D is instantiated from a function parameter pack
/// whose type is a bare type parameter pack (e.g. `Args...`), or a 
/// reference to one (e.g. `Args&...` or `Args&&...`).
```



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:200
+  )cpp",
+   ExpectedHint{"&: ", "fwd"});
+}

As an aside, `&` does not seem like a useful hint to show for `std::forward` -- 
should we try to omit it? (We don't need to do it in this patch as it's not 
really related.)



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:467
+
+TEST(ParameterHints, VariadicVarargs) {
+  assertParameterHints(R"cpp(

I think a variation of this test case where `foo` is also variadic, would be 
valuable to add:

```
template 
void foo(int fixed, Args&&... args);

template 
void bar(Args&&... args) {
  foo(args...);
}

void baz() { 
  bar($fixed[[41]], 42, 43);
}
```

This case does seem to already work with the current patch, and I think it's 
the more common case; I'm happy to defer the varargs one as a less important 
edge case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

I will do my best to take a look this weekend. Your patience is appreciated :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added a comment.

@nridge @sammccall can you give this another look?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:772
+  size_t PackLocation = OptPackLocation.getValue();
+  ArrayRef MatchingParams =
+  Callee->parameters().slice(PackLocation, Parameters.size());

Similar to processCall in InlayHints.cpp, this may have issues with varargs 
functions. Maybe on top of checking for unexpanded pack expression arguments, I 
should add a check `Callee->getNumParams() == Args.size()`. A corresponding 
test fails by not forwarding a fixed parameter right now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 434262.
upsj added a comment.

add test for varargs function called from forwarding function


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/AST.cpp
  clang-tools-extra/clangd/AST.h
  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
@@ -174,6 +174,44 @@
   )cpp");
 }
 
+TEST(ParameterHints, NoNameVariadicDeclaration) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+template 
+void foo(Args&& ...);
+void bar() {
+  foo(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicForwarded) {
+  // No hint for anonymous variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward($fwd[[args]])...); }
+void baz() {
+  bar(42);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"});
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+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(
@@ -186,6 +224,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(
@@ -258,6 +309,291 @@
ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, VariadicForwardedConstructor) {
+  // Name hint for variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward($fwd[[args]])...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"},
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwardedNewConstructor) {
+  // Name hint for variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward($fwd[[args]])...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"},
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwarded) {
+  // Name for variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int a);
+template 
+void bar(Args&&... args) { return foo(std::forward($fwd[[args]])...); }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"},
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlain) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int a);
+

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 434257.
upsj added a comment.

This is now ready to review, only needed to fix a formatting issue


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/AST.cpp
  clang-tools-extra/clangd/AST.h
  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
@@ -174,6 +174,44 @@
   )cpp");
 }
 
+TEST(ParameterHints, NoNameVariadicDeclaration) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+template 
+void foo(Args&& ...);
+void bar() {
+  foo(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicForwarded) {
+  // No hint for anonymous variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward($fwd[[args]])...); }
+void baz() {
+  bar(42);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"});
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+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(
@@ -186,6 +224,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(
@@ -258,6 +309,276 @@
ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, VariadicForwardedConstructor) {
+  // Name hint for variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward($fwd[[args]])...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"},
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwardedNewConstructor) {
+  // Name hint for variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward($fwd[[args]])...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"},
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwarded) {
+  // Name for variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int a);
+template 
+void bar(Args&&... args) { return foo(std::forward($fwd[[args]])...); }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"},
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlain) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int a);
+

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 433732.
upsj added a comment.

- fix varargs, again :)
- remove parameter names entirely if they occur multiple times


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/AST.cpp
  clang-tools-extra/clangd/AST.h
  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
@@ -174,6 +174,44 @@
   )cpp");
 }
 
+TEST(ParameterHints, NoNameVariadicDeclaration) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+template 
+void foo(Args&& ...);
+void bar() {
+  foo(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicForwarded) {
+  // No hint for anonymous variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward($fwd[[args]])...); }
+void baz() {
+  bar(42);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"});
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+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(
@@ -186,6 +224,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(
@@ -258,6 +309,276 @@
ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, VariadicForwardedConstructor) {
+  // Name hint for variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward($fwd[[args]])...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"},
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwardedNewConstructor) {
+  // Name hint for variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward($fwd[[args]])...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"},
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwarded) {
+  // Name for variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int a);
+template 
+void bar(Args&&... args) { return foo(std::forward($fwd[[args]])...); }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"},
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlain) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+void 

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 432900.
upsj marked an inline comment as done.
upsj added a comment.

Thanks Nathan, that was exactly it. I thought I looked at getTypePtr() 
directly, but I must have been mistaken.
After a small fix (only adding inlay hints until the first unexpanded pack 
expression, it seems to work now).

Short explanation of what is going on: I split the parameters into head, pack 
and tail where pack are all parameters matching the variadic template type 
parameter we are expanding.
This way, I need to make no allocations inside the visitor.

The only thing to be fixed is a `make_tuple`-like call, where each parameter is 
called `head`, probably simple deduplication makes most sense.


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/AST.cpp
  clang-tools-extra/clangd/AST.h
  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
@@ -174,6 +174,44 @@
   )cpp");
 }
 
+TEST(ParameterHints, NoNameVariadicDeclaration) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+template 
+void foo(Args&& ...);
+void bar() {
+  foo(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicForwarded) {
+  // No hint for anonymous variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward($fwd[[args]])...); }
+void baz() {
+  bar(42);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"});
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+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(
@@ -186,6 +224,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(
@@ -258,6 +309,276 @@
ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, VariadicForwardedConstructor) {
+  // Name hint for variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward($fwd[[args]])...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"},
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwardedNewConstructor) {
+  // Name hint for variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward($fwd[[args]])...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"},
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwarded) {
+  // Name for variadic parameter
+  // This 

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D124690#3542961 , @nridge wrote:

> I would try using getPointeeTypeAsWritten() 
> 
>  instead and see if that helps.

Indeed, this change makes two tests (VariadicPlainNewConstructor and 
VariadicForwardedNewConstructor) pass. I assume the other failures are due to 
other issues in the patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Haven't had a chance to try it yet, but based on a quick glance, my suspicion 
is that the problem is the use of `ReferenceType::getPointeeType()`, which may 
do more unwrapping than we want (its implementation contains a loop 
).

I would try using getPointeeTypeAsWritten() 

 instead and see if that helps.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 432518.
upsj added a comment.

Of course, I forgot to create a new diff. There is some debug output and logic 
errors for some of the tests, but the simple ones already show the issue.
The function in question is `getPackTemplateParameter`, which provides similar 
functionality to `isExpandedParameter` before


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/AST.cpp
  clang-tools-extra/clangd/AST.h
  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
@@ -174,6 +174,44 @@
   )cpp");
 }
 
+TEST(ParameterHints, NoNameVariadicDeclaration) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+template 
+void foo(Args&& ...);
+void bar() {
+  foo(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicForwarded) {
+  // No hint for anonymous variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward($fwd[[args]])...); }
+void baz() {
+  bar(42);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"});
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+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(
@@ -186,6 +224,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(
@@ -258,6 +309,276 @@
ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, VariadicForwardedConstructor) {
+  // Name hint for variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward($fwd[[args]])...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"},
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwardedNewConstructor) {
+  // Name hint for variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward($fwd[[args]])...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"&: ", "fwd"},
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwarded) {
+  // Name for variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int a);
+template 
+void bar(Args&&... args) { return foo(std::forward($fwd[[args]])...); }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D124690#3540704 , @upsj wrote:

> Example where this pops up:
>
>   cpp
>   namespace std { template  T&& forward(T&); }
>   struct S { S(int a); };
>   template 
>   T bar(Args&&... args) { return T{std::forward($fwd[[args]])...}; }
>   void baz() {
> int b;
> bar($param[[b]]);
>   }

I'm happy to take a look at this, but I would need some updated code to run 
(including in particular the update to the call site that currently uses 
`getNonReferenceType()`)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added a comment.

I've hit a roadblock I'm not sure how to proceed with. It seems that in some 
cases, a `ParmVarDecl` of an instantiated function template indeed doesn't 
preserve the `SubstTemplateTypeParmType` type sugar allowing me to check which 
template parameter it was instantiated from. This might be related to reference 
collapsing on type deduction, since `Args...` preserves the sugar, but 
`Args&&...` doesn't. Without this ability to distinguish between expanded packs 
and normal parameters, I risk recursing into other functions and pulling up 
unrelated type names. Any ideas how to proceed here? Essentially, it's the same 
issue that @nridge already provided a partial solution for by not using 
`getNonReferenceType()`, but more complex. I would really like to avoid having 
to reconstruct parameter indices from arbitrarily many levels of template 
parameters packs from surrounding scopes.

Example where this pops up:

  cpp
  namespace std { template  T&& forward(T&); }
  struct S { S(int a); };
  template 
  T bar(Args&&... args) { return T{std::forward($fwd[[args]])...}; }
  void baz() {
int b;
bar($param[[b]]);
  }


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added a comment.

Yes, I think that's a good summary. Only a small clarification:

> we could query separately where each arg is forwarded to. For multiple 
> forwards, the recursive step is "what param is arg N of this callee 
> ultimately bound to"

I was thinking of implementing this as "what parameters are the following 
arguments bound to in a direct call", which doesn't have the aforementioned 
efficiency issues, and is basically a single recursion level of what I had 
previously implemented. Since (at least afaik) there is no way to have a 
parameter pack split up between different function calls (at least from the AST 
standpoint where we here only allow std::forward or plain parameters), the 
input would be a FunctionDecl and vector/set of ParmVarDecl and the output 
would be a map ParmVarDecl -> ParmVarDecl plus an Optional 
telling us whether we need to recurse further. Unrolling the recursion would 
also make it easier to limit the depth to which we unpack forwarded parameters.




Comment at: clang-tools-extra/clangd/InlayHints.cpp:261
+private:
+  void handleParmVarDeclName(const FunctionDecl *Callee, size_t I) {
+const auto *Param = Callee->getParamDecl(I);

sammccall wrote:
> upsj wrote:
> > sammccall wrote:
> > > Unless I'm missing something, going looking in the redecls of the 
> > > function for a parameter name doesn't seem in scope for this patch.
> > > 
> > > We don't support it in inlay hints elsewhere, and it's not clear it has 
> > > anything to do with forwarding functions.
> > > Maybe the added complexity is justifiable if this logic can be shared 
> > > with different functions (hover, signature help) but I don't think it 
> > > belongs in this patch.
> > This was already functionality previously available in 
> > `chooseParameterNames`, I thought I would need to do the same thing here, 
> > but turns out that I can get the `FunctionDecl` from a `ParmVarDecl`, so 
> > this can stay in its previous place.
> You're right, sorry!
> I think the the version we have is pretty opportunistic: this is a couple of 
> lines of code, so why not?
> I don't think we should aim for parity with it, but rather just handle 
> whatever cases are both useful & trivial to implement (in this patch)
> 
> > I can get the FunctionDecl from a ParmVarDecl, so this can stay in its 
> > previous place.
> This sounds good to me - means we can drop this map right? That was the main 
> piece I was concerned with.
Yes, we can do the same in the InlayHintVisitor, though it becomes a bit more 
complex there, since we don't have the index of the argument, which might 
require a linear search, as different forwarded ParmVarDecls can come from 
different FunctionDecls


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D124690#3530272 , @upsj wrote:

> I will update this to split up the control flow into a visitor just 
> collecting a single recursion level and the high-level mapping. A general 
> question here would be whether we should do the resolution by parameter or by 
> parameter pack. Doing it by parameter (like I did it right now) is much 
> easier for inlay hints, especially if the pack gets split up into different 
> parts, but that may not necessarily be the case for template functions and 
> more fuzzy cases like signature help/autocomplete.

I'm not sure I have perfectly grasped the distinction you're drawing, but IIUC 
the dilemma:

- we could query separately where each arg is forwarded to. For multiple 
forwards, the recursive step is "what param is arg N of this callee ultimately 
bound to"
- we could query where a range of args is forwarded to. The recursive step is 
"what params are args [N-M] of this callee ultimately bound to"

Seems like the first is simpler but the second is more efficient. The second 
also more clearly ensures we don't give inconsistent results for adjacent 
parameters.
The approach you take now where you walk->cache->query does manage to combine 
the best of both (though I find it complex in other ways).
This is a tricky tradeoff. My guess is that it's fine to start with the simple 
param-by-param query, and leave a FIXME on the recursive function that querying 
a range at a time would be more efficient for large packs.




Comment at: clang-tools-extra/clangd/InlayHints.cpp:261
+private:
+  void handleParmVarDeclName(const FunctionDecl *Callee, size_t I) {
+const auto *Param = Callee->getParamDecl(I);

upsj wrote:
> sammccall wrote:
> > Unless I'm missing something, going looking in the redecls of the function 
> > for a parameter name doesn't seem in scope for this patch.
> > 
> > We don't support it in inlay hints elsewhere, and it's not clear it has 
> > anything to do with forwarding functions.
> > Maybe the added complexity is justifiable if this logic can be shared with 
> > different functions (hover, signature help) but I don't think it belongs in 
> > this patch.
> This was already functionality previously available in 
> `chooseParameterNames`, I thought I would need to do the same thing here, but 
> turns out that I can get the `FunctionDecl` from a `ParmVarDecl`, so this can 
> stay in its previous place.
You're right, sorry!
I think the the version we have is pretty opportunistic: this is a couple of 
lines of code, so why not?
I don't think we should aim for parity with it, but rather just handle whatever 
cases are both useful & trivial to implement (in this patch)

> I can get the FunctionDecl from a ParmVarDecl, so this can stay in its 
> previous place.
This sounds good to me - means we can drop this map right? That was the main 
piece I was concerned with.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:314
+if (auto *FuncCandidate = dyn_cast_or_null(Candidate)) {
+  if (FuncCandidate->getNumParams() == D->getNumArgs()) {
+if (MatchingDecl) {

upsj wrote:
> There is probably more generic functionality available for this?
Not AFAIK. There's a bunch of rich info about overload sets and their quality, 
but it's discarded after Sema and not preserved in the AST I think.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj planned changes to this revision.
upsj marked 15 inline comments as done.
upsj added a comment.

I will update this to split up the control flow into a visitor just collecting 
a single recursion level and the high-level mapping. A general question here 
would be whether we should do the resolution by parameter or by parameter pack. 
Doing it by parameter (like I did it right now) is much easier for inlay hints, 
especially if the pack gets split up into different parts, but that may not 
necessarily be the case for template functions and more fuzzy cases like 
signature help/autocomplete.




Comment at: clang-tools-extra/clangd/InlayHints.cpp:261
+private:
+  void handleParmVarDeclName(const FunctionDecl *Callee, size_t I) {
+const auto *Param = Callee->getParamDecl(I);

sammccall wrote:
> Unless I'm missing something, going looking in the redecls of the function 
> for a parameter name doesn't seem in scope for this patch.
> 
> We don't support it in inlay hints elsewhere, and it's not clear it has 
> anything to do with forwarding functions.
> Maybe the added complexity is justifiable if this logic can be shared with 
> different functions (hover, signature help) but I don't think it belongs in 
> this patch.
This was already functionality previously available in `chooseParameterNames`, 
I thought I would need to do the same thing here, but turns out that I can get 
the `FunctionDecl` from a `ParmVarDecl`, so this can stay in its previous place.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Glad this is working! It looks exciting...

My high level comments are:

- the core "what are the ultimate param vars for this call" function is large 
and reusable enough that we should give this a public interface in AST.h
- the implementation does a few "extra" things and it's not clear they're worth 
their complexity cost
- the RecursiveASTVisitor is a bit of a "god object" at the moment, and should 
probably be split up

(Sorry if the comments below are duplicative)




Comment at: clang-tools-extra/clangd/InlayHints.cpp:189
 
+bool isExpandedParameter(const ParmVarDecl *Param) {
+  auto PlainType = Param->getType().getNonReferenceType();

nit: isPackExpandedParameter
(usually "expanded" means macro expansion)



Comment at: clang-tools-extra/clangd/InlayHints.cpp:197
+
+class ForwardingParameterVisitor
+: public RecursiveASTVisitor {

I think this visitor has too many responsibilities:
 - it controls the lifetime of caches for reuse of partial results. (FWIW, I'm 
skeptical there's any serious benefit of this on real code, and would prefer to 
avoid paying any complexity costs for it)
 - it begins a traversal over function bodies, hooking interesting nodes to 
find forwarding calls and analyzing them
 - it owns the queues that are used for recursive resolution. (Again, it's 
unclear if these are providing value, but it's hard to separate them out from 
the design).
 - (via inheritance) it implements the mechanics of traversal itself

Could you try to reduce the scope of the visitor down to its very minimum: the 
inherited implementation of traversal + recording of forwarding calls?

e.g.
```
// Traverses a function body, recording locations where a particular
// parameter pack was forwarded to another call.
class FindForwardingCalls : public RecursiveASTVisitor {
  FindForwardingCalls(ParmVarDecl Pack);

  struct ForwardingCall {
FunctionDecl *Callee;
unsigned PackOffset; // e.g. 0 for make_unique, 1 for map::try_emplace
  };

  vector> ForwardingCalls;
};
```

Then the other pieces can be decoupled and structured in ways that make the 
most sense individually.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:201
+  void
+  resolveForwardedParameters(const FunctionDecl *Callee,
+ llvm::SmallVector ) {

this is only called once, and the Params is always Callee->params().

Can this be a function instead that takes only Callee and returns the params?

If possible, I'd declare this function in `AST.h` and hide the 
RecursiveASTVisitor in `AST.cpp`.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:208
+// If the parameter is part of an expanded pack and not yet resolved
+if (/*isExpandedParameter(Param) && */
+ForwardedParams.find(Param) == ForwardedParams.end()) {

nridge wrote:
> nridge wrote:
> > upsj wrote:
> > > This needs to be fixed, see `ParameterHints.VariadicPlain` vs. 
> > > `ParameterHints.VariadicForwarded` if uncommented - I'd need some input 
> > > from somebody with more knowledge about the AST
> > It looks like `isExpandedParameter()` relies on the 
> > `SubstTemplateTypeParmType` type sugar being present in the ParmVarDecl's 
> > type, but in some cases, the ParmVarDecl's type points to the canonical 
> > type directly.
> > 
> > I'm not sure what sort of guarantees the AST intends to make about the 
> > presence of type sugar. Based on past experience with Eclipse CDT, it's 
> > very easy to lose type sugar and maintaining it in all the right places 
> > takes some effort.
> Upon further investigation, it looks like the ParmVarDecl is retaining the 
> type sugar fine, it's the `getNonReferenceType()` call in 
> `isExpandedParameter()` that loses it.
> 
> What happens with perfect forwarding when the argument is an lvalue is a bit 
> subtle. In this testcase:
> 
> ```
> template 
> void bar(Args&&... args) { return foo(std::forward(args)...); }
> void baz() {
>   int b;
>   bar($param[[b]]);
> }
> ```
> 
> the template argument that `bar()` is instantiated with is `Args = [int &]`. 
> Substituting into `Args&&`, that then becomes `int& &&` which collapses into 
> `int&`, leaving the instantiated parameter type an lvalue reference type.
> 
> Clang does in fact model this accurately, which means the type structure is:
> 
> ```
> BuiltinType
>   ReferenceType
> SubstTemplateTypeParmType
>   ReferenceType
> ```
> 
> The outer reference type is the `&&` that's outside the `Args`, the 
> `SubstTemplateTypeParmType` reflects the substitution `Args = int&`, and the 
> inner `ReferenceType` is the `int&`.
> 
> The problem is, `getNonReferenceType()` unpacks _all_ the reference types, 
> skipping past the `SubstTemplateTypeParmType` and giving you the 
> `BuiltinType`.
Ah, great catch.

I think it's fine to assume that ParmVarDecls in 

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:208
+// If the parameter is part of an expanded pack and not yet resolved
+if (/*isExpandedParameter(Param) && */
+ForwardedParams.find(Param) == ForwardedParams.end()) {

nridge wrote:
> upsj wrote:
> > This needs to be fixed, see `ParameterHints.VariadicPlain` vs. 
> > `ParameterHints.VariadicForwarded` if uncommented - I'd need some input 
> > from somebody with more knowledge about the AST
> It looks like `isExpandedParameter()` relies on the 
> `SubstTemplateTypeParmType` type sugar being present in the ParmVarDecl's 
> type, but in some cases, the ParmVarDecl's type points to the canonical type 
> directly.
> 
> I'm not sure what sort of guarantees the AST intends to make about the 
> presence of type sugar. Based on past experience with Eclipse CDT, it's very 
> easy to lose type sugar and maintaining it in all the right places takes some 
> effort.
Upon further investigation, it looks like the ParmVarDecl is retaining the type 
sugar fine, it's the `getNonReferenceType()` call in `isExpandedParameter()` 
that loses it.

What happens with perfect forwarding when the argument is an lvalue is a bit 
subtle. In this testcase:

```
template 
void bar(Args&&... args) { return foo(std::forward(args)...); }
void baz() {
  int b;
  bar($param[[b]]);
}
```

the template argument that `bar()` is instantiated with is `Args = [int &]`. 
Substituting into `Args&&`, that then becomes `int& &&` which collapses into 
`int&`, leaving the instantiated parameter type an lvalue reference type.

Clang does in fact model this accurately, which means the type structure is:

```
BuiltinType
  ReferenceType
SubstTemplateTypeParmType
  ReferenceType
```

The outer reference type is the `&&` that's outside the `Args`, the 
`SubstTemplateTypeParmType` reflects the substitution `Args = int&`, and the 
inner `ReferenceType` is the `int&`.

The problem is, `getNonReferenceType()` unpacks _all_ the reference types, 
skipping past the `SubstTemplateTypeParmType` and giving you the `BuiltinType`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:208
+// If the parameter is part of an expanded pack and not yet resolved
+if (/*isExpandedParameter(Param) && */
+ForwardedParams.find(Param) == ForwardedParams.end()) {

upsj wrote:
> This needs to be fixed, see `ParameterHints.VariadicPlain` vs. 
> `ParameterHints.VariadicForwarded` if uncommented - I'd need some input from 
> somebody with more knowledge about the AST
It looks like `isExpandedParameter()` relies on the `SubstTemplateTypeParmType` 
type sugar being present in the ParmVarDecl's type, but in some cases, the 
ParmVarDecl's type points to the canonical type directly.

I'm not sure what sort of guarantees the AST intends to make about the presence 
of type sugar. Based on past experience with Eclipse CDT, it's very easy to 
lose type sugar and maintaining it in all the right places takes some effort.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

One more testcase:

  template 
  void foo(Args...);
  
  template 
  void bar(Args... args) {
foo(args...);
  }
  
  template 
  void foo(Args... args) {
bar(args...);
  }
  
  int main() {
foo(1, 2);
  }

Sure, this is a stack overflow at runtime, but there's no excuse for it to be 
one in clangd :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Another test case that comes to mind is:

  void f1(int a, int b);
  void f2(int c, int d);
  
  template 
  void foo(Args... args) {
if (cond) {
  f1(args...);
} else {
  f2(args...);
}
  }
  
  int main() {
foo(1, 2);
  }

I guess in this case it will use the parameters names from `f1`, because that's 
the first call that appears in the function body in traversal order?

I think that's fine, functions written like this are probably not very common. 
Still good to add to the test suite to document the behaviour.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Had a read through this. I'm still digesting it, but the high-level approach 
seems reasonable to me.

Could we add a test case for the recursive scenario that came up during chat:

  void foo();
  
  template 
  void foo(Head head, Tail... tail) {
foo(tail...);
  }
  
  int main() {
foo(1, 2, 3);
  }
  `

(even if the behaviour on this testcase isn't what we want yet, it's useful to 
have it in the test suite as a reminder)




Comment at: clang-tools-extra/clangd/InlayHints.cpp:344
+
+  const DeclRefExpr *unpackArgument(const Expr *E) {
+E = unpackImplicitCast(E);

unwrap? "unpack" sounds like something related to parameter packs



Comment at: clang-tools-extra/clangd/InlayHints.cpp:374
+  // have one in the definition, store this mapping here.
+  llvm::DenseMap ParamDeclToDef;
+};

I think it would be more readable if the state that persists across calls 
(TraversedFunctions, ForwardedParams, ParamDeclToDef) would be separate from 
the state that does not (UnresolvedParams, TraversalQueue).

A concrete suggestion:

 * Factor the first set out into a `ParameterMappingCache` struct
 * Do not keep a `ForwardingParameterVisitor` as a member of 
`InlayHintVisitor`, only the `ParameterMappingCache`
 * Create the `ForwardingParameterVisitor` as a local for each call, and pass 
it a reference to the `ParameterMappingCache` in the constructor


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added a comment.

It seems like except for the caveats I listed before, all the obvious cases 
seem to work: `make_unique`, `make_shared`, `emplace_back` with exact type 
matches. One point that still needs some work is if the parameter needs to be 
converted inside one of the forwarding functions (probably just needs another 
`unpack` inside ForwardingParameterVisitor), as well as a way to remove 
duplicate parameters that come from recursive templates like `std::tuple`. One 
obvious way would be removing inlay hints for duplicate parameters altogether, 
but that may not be enough/too heuristic? Alternatively, we could inspect the 
template instantiation pattern.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:208
+// If the parameter is part of an expanded pack and not yet resolved
+if (/*isExpandedParameter(Param) && */
+ForwardedParams.find(Param) == ForwardedParams.end()) {

This needs to be fixed, see `ParameterHints.VariadicPlain` vs. 
`ParameterHints.VariadicForwarded` if uncommented - I'd need some input from 
somebody with more knowledge about the AST



Comment at: clang-tools-extra/clangd/InlayHints.cpp:314
+if (auto *FuncCandidate = dyn_cast_or_null(Candidate)) {
+  if (FuncCandidate->getNumParams() == D->getNumArgs()) {
+if (MatchingDecl) {

There is probably more generic functionality available for this?



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:447
+namespace std { template  T&& forward(T&); }
+void *operator new(unsigned long, void *);
+struct S {

This is not portable, but I don't have access to size_t


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 427930.
upsj added a comment.

attempt to fix the patch issue


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 
+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  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward(args)...); }
+void baz() {
+  bar(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+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  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($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  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($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  T&& forward(T&); }
+void foo(int a);
+template 
+void bar(Args&&... args) { return foo(std::forward(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 
+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 

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

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 
+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  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward(args)...); }
+void baz() {
+  bar(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+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  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($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  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($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  T&& forward(T&); }
+void foo(int a);
+template 
+void bar(Args&&... args) { return foo(std::forward(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 
+void bar(Args&&... args) { return foo(args...); }
+void baz() {
+  bar($param[[42]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicSplitRecursive) {
+  // Name 

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 427861.
upsj added a comment.

fix iterator invalidation issue, handle UnresolvedLookupExpr and test recursive 
and split variadic lookup


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 
+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  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward(args)...); }
+void baz() {
+  bar(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+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,168 @@
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  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($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  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($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  T&& forward(T&); }
+void foo(int a);
+template 
+void bar(Args&&... args) { return foo(std::forward(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 
+void bar(Args&&... args) { return foo(args...); }
+void baz() {
+  bar($param[[42]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicSplitRecursive) {
+  // Name for 

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 427855.
upsj added a comment.

updated patch baseline


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 
+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  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward(args)...); }
+void baz() {
+  bar(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+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(
@@ -254,6 +291,123 @@
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  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($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  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($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  T&& forward(T&); }
+void foo(int a);
+template 
+void bar(Args&&... args) { return foo(std::forward(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 
+void bar(Args&&... args) { return foo(args...); }
+void baz() {
+  bar($param[[42]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+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  T&& forward(T&); }
+void foo(int a);
+template 
+void bar(Args&&... args) { return foo(std::forward(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 
+void bar(Args&&... args) { return foo(args...); }
+void baz() {
+  int a;
+  

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 427853.
upsj marked an inline comment as done.
upsj added a comment.

Work around the currently broken isExpandedParameter, also fix the broken diff


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 
+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  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward(args)...); }
+void baz() {
+  bar(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+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(
@@ -254,6 +291,123 @@
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  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($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  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($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  T&& forward(T&); }
+void foo(int a);
+template 
+void bar(Args&&... args) { return foo(std::forward(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 
+void bar(Args&&... args) { return foo(args...); }
+void baz() {
+  bar($param[[42]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+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  T&& forward(T&); }
+void foo(int a);
+template 
+void bar(Args&&... args) { return foo(std::forward(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 
+ 

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj marked 10 inline comments as done.
upsj added a comment.

This almost works now, the only thing that needs to be fixed is 
`isExpandedParameter`, which for some reason picks up the first call, but not 
the second: (Compare `ParameterHints.Forwarded` and 
`ParameterHints.VariadicPlain`)

  cpp
  void foo(int a);
  template 
  void bar(Args&&... args) { return foo(args...); }
  void baz() {
int b;
bar(42); // This parameter is recognized as expanded
bar(b); // This one doesn't, so its inlay hint is @args: instead of a:
  }




Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:178
+  // No hint for anonymous variadic parameter
+  // The prototype is not correct, but is converted into builtin anyways.
+  assertParameterHints(R"cpp(

nridge wrote:
> What does "converted into builtin" mean here?
I am talking about std::forward being a builtin function, which can be detected 
by its BuiltinID, so it doesn't matter if the signature doesn't match 100%.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 427851.
upsj added a comment.
Herald added a subscriber: javed.absar.

use an RecursiveASTVisitor instead of ASTMatcher


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/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -101,7 +101,9 @@
   auto ModuleCacheDeleter = llvm::make_scope_exit(
   std::bind(deleteModuleCache, CI->getHeaderSearchOpts().ModuleCachePath));
   return clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
-  /*StoreInMemory=*/true, PreambleCallback);
+  /*StoreInMemory=*/true,
+  /*ParseForwardingFunctions=*/false,
+  PreambleCallback);
 }
 
 ParsedAST TestTU::build() const {
@@ -115,9 +117,10 @@
   auto ModuleCacheDeleter = llvm::make_scope_exit(
   std::bind(deleteModuleCache, CI->getHeaderSearchOpts().ModuleCachePath));
 
-  auto Preamble = clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
-   /*StoreInMemory=*/true,
-   /*PreambleCallback=*/nullptr);
+  auto Preamble =
+  clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
+   /*StoreInMemory=*/true,
+   /*PreambleCallback=*/false, nullptr);
   auto AST = ParsedAST::build(testPath(Filename), Inputs, std::move(CI),
   Diags.take(), Preamble);
   if (!AST.hasValue()) {
Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -173,7 +173,8 @@
   TU.AdditionalFiles["c.h"] = "";
   auto PI = TU.inputs(FS);
   auto BaselinePreamble = buildPreamble(
-  TU.Filename, *buildCompilerInvocation(PI, Diags), PI, true, nullptr);
+  TU.Filename, *buildCompilerInvocation(PI, Diags), PI,
+  /*StoreInMemory=*/true, /*ParseForwardingFunctions=*/false, nullptr);
   // We drop c.h from modified and add a new header. Since the latter is patched
   // we should only get a.h in preamble includes.
   TU.Code = R"cpp(
Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -497,7 +497,8 @@
   auto Inputs = TU.inputs(FS);
   auto CI = buildCompilerInvocation(Inputs, Diags);
   auto EmptyPreamble =
-  buildPreamble(testPath("foo.cpp"), *CI, Inputs, true, nullptr);
+  buildPreamble(testPath("foo.cpp"), *CI, Inputs, /*StoreInMemory=*/true,
+/*PreambleCallback=*/false, nullptr);
   ASSERT_TRUE(EmptyPreamble);
   EXPECT_THAT(EmptyPreamble->Includes.MainFileIncludes, IsEmpty());
 
@@ -540,7 +541,8 @@
   auto Inputs = TU.inputs(FS);
   auto CI = buildCompilerInvocation(Inputs, Diags);
   auto BaselinePreamble =
-  buildPreamble(testPath("foo.cpp"), *CI, Inputs, true, nullptr);
+  buildPreamble(testPath("foo.cpp"), *CI, Inputs, /*StoreInMemory=*/true,
+/*PreambleCallback=*/false, nullptr);
   ASSERT_TRUE(BaselinePreamble);
   EXPECT_THAT(BaselinePreamble->Includes.MainFileIncludes,
   ElementsAre(testing::Field(::Written, "")));
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 
+void foo(Args&& ...);
+void bar() {
+  foo(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, 

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:255
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}

upsj wrote:
> sammccall wrote:
> > upsj wrote:
> > > 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`.
> > The AST here of the instantiated bar looks like
> > ```
> > -FunctionDecl  line:5:4 used bar 'S *(int &&)'
> >   |-TemplateArgument type 'S'
> >   |-TemplateArgument pack
> >   | `-TemplateArgument type 'int'
> >   |-ParmVarDecl  col:18 used args 'int &&'
> >   `-CompoundStmt 
> > `-ReturnStmt 
> >   `-CXXNewExpr  'S *' Function 'operator new' 'void 
> > *(unsigned long)'
> > `-CXXConstructExpr  'S':'S' 'void (int)' list
> >   `-ImplicitCastExpr  'int':'int' 
> > `-DeclRefExpr  'int':'int' lvalue ParmVar 'args' 'int 
> > &&'
> > ```
> > 
> > So there's no CXXTemporaryObjectExpr (the value here is a pointer), but 
> > should be a CXXConstructExpr. 
> > 
> > Are you sure you're traversing bar's instantiation/specializaion, as 
> > opposed to its templated/generic FunctionDecl? The AST of the templated 
> > FunctionDecl indeed has an InitListExpr in place of the CXXConstructExpr 
> > because it's not resolved yet.
> It's quite possible this is related to the visitor not/only inconsistently 
> traversing template instantiations due to shouldVisitTemplateInstantiations 
> returning false. But I need to implement the new approach first, anyways.
Ah, I think I see the confusion.

We don't want to gather all the information in a single traversal of the whole 
AST. Such a traversal would need to walk all instantiated templates including 
those from headers, which is far too much/too slow.

Instead, you want to analyze particular forwarding functions by walking through 
their bodies looking for the forwarding call. RecursiveASTVisitor is the tool 
for this job - you just call TraverseDecl(FunctionDecl) to limit the scope. The 
visitor should not visit template instantiations, but the traversal can still 
be rooted at an instantiation!

So something roughly like:

```
Optional> ultimateParams(FunctionDecl* Callee) {
  if (!Callee || !isa(Callee->getType()))
return None;
  if (not instantiated from template || Pattern doesn't use a pack)
return Callee->parameters();

  for (P : Pattern->parameters()) {
if (!P->isPack())
  Result.push_back(Callee->parameters[Pos++]);
else {
  if (PackExpansionSize = ...) {
if (ForwardedParams = forwardedParams(Callee, P))
  Result.append(*ForwardedParams);
else // failed, no param info for forwarded args
  Result.append(nullptr, PackExpansionSize);
Pos += PackExpansionSize;
  }
}
  }
}

Optional> forwardedParams(FunctionDecl *Callee, 
ParmVarDecl *Pack) {
  class ForwardFinder : RAV {
bool VisitCallExpr(CallExpr *E) {
  if (E->params() expands Pack) {
if (CallParams = ultimateParams(E->getCallee()->getAsFunctionDecl()))
  Result = CallParams.slice(section corresponding to Pack);
  }
}
  };
  ForwardFinder Finder;
  if (Callee->hasBody())
return Finder.VisitCallExpr(Callee);  
}
```

you could also memoize with a map, but I suspect simple recursion is going to 
be good enough


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added a comment.

> ! In D124690#3493246 , @sammccall 
> wrote:
>
> This makes sense to me.
>  This sounds more complicated, but if I'm reading correctly this multi-level 
> forwarding isn't handled in the current version of the patch either?
>  At some point we'll want to extract this logic out so it can be used by 
> hover etc, but not yet.

Yes, currently only direct forwarding works. I originally looked at this when 
inlay hints were not yet available, and it seemed possible to do this for 
signature help as well, only most likely based on the template instantiation 
pattern instead of the instantiated function declaration.




Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:255
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}

sammccall wrote:
> upsj wrote:
> > 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`.
> The AST here of the instantiated bar looks like
> ```
> -FunctionDecl  line:5:4 used bar 'S *(int &&)'
>   |-TemplateArgument type 'S'
>   |-TemplateArgument pack
>   | `-TemplateArgument type 'int'
>   |-ParmVarDecl  col:18 used args 'int &&'
>   `-CompoundStmt 
> `-ReturnStmt 
>   `-CXXNewExpr  'S *' Function 'operator new' 'void 
> *(unsigned long)'
> `-CXXConstructExpr  'S':'S' 'void (int)' list
>   `-ImplicitCastExpr  'int':'int' 
> `-DeclRefExpr  'int':'int' lvalue ParmVar 'args' 'int &&'
> ```
> 
> So there's no CXXTemporaryObjectExpr (the value here is a pointer), but 
> should be a CXXConstructExpr. 
> 
> Are you sure you're traversing bar's instantiation/specializaion, as opposed 
> to its templated/generic FunctionDecl? The AST of the templated FunctionDecl 
> indeed has an InitListExpr in place of the CXXConstructExpr because it's not 
> resolved yet.
It's quite possible this is related to the visitor not/only inconsistently 
traversing template instantiations due to shouldVisitTemplateInstantiations 
returning false. But I need to implement the new approach first, anyways.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D124690#3492981 , @upsj wrote:

> I will take a different approach to the matchers: For each CallExpr involving 
> parameter packs, record the arguments (if they are a ParmVarDecl or 
> stdForward(ParmVarDecl)) and the matching ParmVarDecl of the FunctionDecl in 
> a map. Then I will compact that map to skip intermediate forwarding functions 
> like emplace_back -> allocator::construct -> constructor and in a second pass 
> resolve all previously unresolved forwarded parameters.

This makes sense to me.
This sounds more complicated, but if I'm reading correctly this multi-level 
forwarding isn't handled in the current version of the patch either?
At some point we'll want to extract this logic out so it can be used by hover 
etc, but not yet.




Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:255
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}

upsj wrote:
> 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`.
The AST here of the instantiated bar looks like
```
-FunctionDecl  line:5:4 used bar 'S *(int &&)'
  |-TemplateArgument type 'S'
  |-TemplateArgument pack
  | `-TemplateArgument type 'int'
  |-ParmVarDecl  col:18 used args 'int &&'
  `-CompoundStmt 
`-ReturnStmt 
  `-CXXNewExpr  'S *' Function 'operator new' 'void 
*(unsigned long)'
`-CXXConstructExpr  'S':'S' 'void (int)' list
  `-ImplicitCastExpr  'int':'int' 
`-DeclRefExpr  'int':'int' lvalue ParmVar 'args' 'int &&'
```

So there's no CXXTemporaryObjectExpr (the value here is a pointer), but should 
be a CXXConstructExpr. 

Are you sure you're traversing bar's instantiation/specializaion, as opposed to 
its templated/generic FunctionDecl? The AST of the templated FunctionDecl 
indeed has an InitListExpr in place of the CXXConstructExpr because it's not 
resolved yet.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj planned changes to this revision.
upsj added a comment.

I will take a different approach to the matchers: For each CallExpr involving 
parameter packs, record the arguments (if they are a ParmVarDecl or 
stdForward(ParmVarDecl)) and the matching ParmVarDecl of the FunctionDecl in a 
map. Then I will compact that map to skip intermediate forwarding functions 
like emplace_back -> allocator::construct -> constructor and in a second pass 
resolve all previously unresolved forwarded parameters.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:474
+// the parameter names from the wrapped function
+if (Args.size() > FixedParamCount && Args.size() == Params.size()) {
+  auto ForwardedParams = matchForwardedParams(

upsj wrote:
> nridge wrote:
> > What is the reason for the `Args.size() == Params.size()` condition?
> > 
> > Doesn't this break cases where there is more than one argument matching the 
> > variadic parameter? For example:
> > 
> > ```
> > void foo(int a, int b);
> > template 
> > void bar(Args&&... args) { foo(args...); }
> > int main() {
> >   bar(1, 2);
> > }
> > ```
> > 
> > Here, I expect we'd have `Args.size() == 2` and `Params.size() == 1`.
> > 
> > (We should probably have test coverage for such multiple-arguments cases as 
> > well. We probably don't need it for all combinations, but we should have at 
> > least one test case exercising it.)
> The function we are looking at is an already instantiated template. The check 
> `Args.size() == Params.size()` is only necessary because of va_args
Ah, thanks, I overlooked that. A lot of things in this patch that initially 
confused me make a lot more sense now :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:474
+// the parameter names from the wrapped function
+if (Args.size() > FixedParamCount && Args.size() == Params.size()) {
+  auto ForwardedParams = matchForwardedParams(

nridge wrote:
> What is the reason for the `Args.size() == Params.size()` condition?
> 
> Doesn't this break cases where there is more than one argument matching the 
> variadic parameter? For example:
> 
> ```
> void foo(int a, int b);
> template 
> void bar(Args&&... args) { foo(args...); }
> int main() {
>   bar(1, 2);
> }
> ```
> 
> Here, I expect we'd have `Args.size() == 2` and `Params.size() == 1`.
> 
> (We should probably have test coverage for such multiple-arguments cases as 
> well. We probably don't need it for all combinations, but we should have at 
> least one test case exercising it.)
The function we are looking at is an already instantiated template. The check 
`Args.size() == Params.size()` is only necessary because of va_args


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:388
+// arguments
+auto ForwardedParmMatcher = compoundStmt(forEachDescendant(
+invocation(

upsj wrote:
> 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.
I imagine something like running a `RecursiveASTVisitor` on 
`Callee->getBody()`, and overriding `VisitCallExpr()` and 
`VisitCXXConstructExpr()` to check whether it is a call of the right form.

I don't //think// theres a need to worry about lambdas nested in the body, I 
think we may still be able to get useful parameter names out of them, as in 
e.g.:

```
struct S {
 S(int a, int b);
};

template 
auto Make(Args... args) {
  auto ConstructTask = [&](){ return T(args...); };
  return ConstructTask();
}

int main() {
  auto s = Make(1, 2);
}
```



Comment at: clang-tools-extra/clangd/InlayHints.cpp:474
+// the parameter names from the wrapped function
+if (Args.size() > FixedParamCount && Args.size() == Params.size()) {
+  auto ForwardedParams = matchForwardedParams(

What is the reason for the `Args.size() == Params.size()` condition?

Doesn't this break cases where there is more than one argument matching the 
variadic parameter? For example:

```
void foo(int a, int b);
template 
void bar(Args&&... args) { foo(args...); }
int main() {
  bar(1, 2);
}
```

Here, I expect we'd have `Args.size() == 2` and `Params.size() == 1`.

(We should probably have test coverage for such multiple-arguments cases as 
well. We probably don't need it for all combinations, but we should have at 
least one test case exercising it.)



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:178
+  // No hint for anonymous variadic parameter
+  // The prototype is not correct, but is converted into builtin anyways.
+  assertParameterHints(R"cpp(

What does "converted into builtin" mean here?



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:203
+TEST(ParameterHints, VariadicForwardedConstructor) {
+  // No hint for anonymous variadic parameter
+  // The prototype is not correct, but is converted into builtin anyways.

This comment seems out of sync with the testcase (same for a few others)



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:209
+template 
+T bar(Args&&... args) { return T{$wrapped[[std::forward(args)...]]}; 
}
+void baz() {

The `wrapped` range doesn't seem to be used anywhere (same for a few other 
testcases)



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:270
+  )cpp",
+   ExpectedHint{"a: ", "wrapped"},
+   ExpectedHint{"a: ", "param"});

upsj wrote:
> This is a bit of an unfortunate side-effect of looking at instantiated 
> templates.
Perhaps it would make sense to exclude arguments which are pack expansion 
expressions from getting parameter hints?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Tobias Ribizel via Phabricator via cfe-commits]

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  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 
+void foo(Args&& ...);
+void bar() {
+  foo(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicForwarded) {
+  // No hint for