kadircet added a comment.
In D130260#3671290 <https://reviews.llvm.org/D130260#3671290>, @sammccall wrote:
> driveby thoughts, but please don't block on them.
>
> (if this fix is a heuristic that fixes a common crash but isn't completely
> correct, that may still be worth landing but warrants a fixme)
The fix is not an heuristic. We just make sure we never consider a function
with less parameters than the arguments we have, that way rest of the
assumptions hold (we might still pick a wrong function call, as we could in the
past, but we won't crash).
In addition to that heuristics are improved a little bit to work in presence of
copy/move constructors.
================
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;
----------------
sammccall wrote:
> 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
i agree that we can do things differently here, but with this patch i am mostly
trying to make sure we address the resiliency so that this doesn't bite us in
production (and a couple of easy improvements to the existing cases for common
code patterns).
i'd land this patch as-is and discuss doing these changes in a separate issue
if possible.
================
Comment at: clang-tools-extra/clangd/AST.cpp:833-837
+ auto ParamEnd = It + Parameters.size() - 1;
+ assert(std::distance(Args.begin(), ParamEnd) <
+ std::distance(Args.begin(), Args.end()) &&
+ "Only functions with greater than or equal number of parameters
"
+ "should be checked.");
----------------
upsj wrote:
> I think it would be safer to check this explicitly, since advancing the
> iterator past its end might be UB (depending on the implementation).
i didn't do an explicit check, because we already have that explicit check
inside the call site and doing a possibly linear operation for each argument in
production setup doesn't feel nice
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1468
+ void bar(Args... args) {
+ foo(id($param1[[args]], $param2[[1]])...);
+ }
----------------
sammccall wrote:
> (Just so I understand: these are getting hinted due to the "only
> instantiation" logic, right?)
not only instantiation but "only overload" logic (i suppose that's what you
mean?)
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
