[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<TemplateTypeParmDecl>(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 <typename... B, typename A>
> > 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<size_t> 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<FunctionTemplateDecl*> 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