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
