[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Tobias Ribizel via Phabricator via cfe-commits]

upsj 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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130260/new/

https://reviews.llvm.org/D130260