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

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

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

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

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

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

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

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

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

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 mo

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

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();

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

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

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

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); } sammcc

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

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

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

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

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

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 wrot

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

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/A

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

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/ ht

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

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/ htt

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

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

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

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 the

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

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(

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

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( --

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

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

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

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 matchin

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

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 som

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

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,

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

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 Fi

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

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

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

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:/

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

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

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

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.ll

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

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://revie

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

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-too

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

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()) { -

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

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 funct

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

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...); }

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

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 w

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

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

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

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()) {

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

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()) {

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

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

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

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 resol

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

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

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

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 "wh

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

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 fro

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

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(

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

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

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

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

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

nridge added a comment. In D124690#3542961 , @nridge wrote: > I would try using getPointeeTypeAsWritten() > > instead and see if that helps. I

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

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 expre

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

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 File

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

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-ext

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

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/cl

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

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 i

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

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-commit

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

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-c

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

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/

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

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

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

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

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

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 traili

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

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:

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

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 documenta

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

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

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

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/D1246