sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/AST.cpp:832
continue;
+ // Check that this expands all the way until the last parameter.
+ auto ParamEnd = It + Parameters.size() - 1;
----------------
upsj wrote:
> sammccall wrote:
> > The essence of this test seems to be "if we can't rule it out by counting
> > parameters, it must be right" which... doesn't seem very robust? Am I
> > missing something?
> >
> > AIUI the idea here is that
> > A) `foo(pack...)` is "expanded all the way" into consecutive arguments
> > B) `foo(pack)...` is not considered forwarding for our purposes
> > C) we want to treat `foo(bar(pack)...)` specially if `bar` is "just a cast"
> > like move or forward
> >
> > The difference between cases A and B aren't about counting arguments
> > though, they're about what operations occur intercede between `pack` and
> > the `...` operator. (i.e. `foo((&pack)...)` is fine for the same reason
> > `forward` is.
> >
> > This seems like something best handled by:
> > - finding the `PackExpansionExpr` controlling `pack` in the
> > non-instantiated template AST
> > - walking down to try to find `pack`
> > - continue as we walk through e.g. parens or std::forward
> > - bail out if we walk through something unknown (e.g. another function
> > call)
> >
> > Does this seem sensible/feasible?
> > (Walking up from `pack` is nicer but the AST doesn't support walking up
> > well...)
> This goes a bit more generally into the question how to approach parameter
> packs across clangd. For inlay hints and hover, we are already looking at
> instantiated templates (is that true in all cases? Haven't dug in yet), so
> implementing forwarding there was straightforward. For signature help and
> code completion, we'd probably need to go via the uninstantiated template
> (and probably lose a bit of type information on the way, e.g. reference vs.
> value, l-value vs. r-value). If we wanted a generic way to deal with
> forwarding, the instantiation pattern would be my first choice, but I didn't
> have the time to go down this rabbit hole yet.
> Unless there is a nice way to map between template and instantiation AST, I
> don't think combining them is feasible.
>
> On the heuristic itself: Is there a way in C++ to "partially" unpack a
> parameter pack without this showing up in the AST? I am not aware of any,
> which is why the test "do the first and last parameters appear in a matching
> range of the arguments" seems sufficient and necessary in all but
> single-entry parameter packs.
> Unless there is a nice way to map between template and instantiation AST, I
> don't think combining them is feasible.
Fair enough, I don't see one.
> On the heuristic itself: Is there a way in C++ to "partially" unpack a
> parameter pack without this showing up in the AST?
Not AFAIK, the is either expanded at "this level" yielding sibling args, or at
"a higher level" yielding a single arg.
So maybe the "first & last" test is actually robust, but it would need some
justification in comments.
There's an exception I think, if the pack size is one it falls over:
```
bar(int bar0);
bar(int bar0, int bar1);
bar(int bar0i, int bar1, int bar2);
template <class...Args>
void foo(Args...args) {
bar(args...); // should yield labels when expanded
bar(args)...; // should not yield labels when expanded
}
// but when there's one arg (foo<int>) the two have the same AST
```
I think it's OK to just get this one wrong
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130260/new/
https://reviews.llvm.org/D130260
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
