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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits