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


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:541
 
+    // Remove parameter names that occur multiple times completely.
+    llvm::StringMap<size_t> NameLastSeen;
----------------
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.


================
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 <typename... Args>
>     void foo(int fixed, Args&&... args);
> 
>     template <typename... Args>
>     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