sammccall added a comment.
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)
================
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;
----------------
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...)
================
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).
Right, this is definitely UB as written.
FWIW I find this easier to understand in classic bounds-check form:
if ((It - Args.begin()) + Parameters.size() >= Args.size())
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1468
+ void bar(Args... args) {
+ foo(id($param1[[args]], $param2[[1]])...);
+ }
----------------
(Just so I understand: these are getting hinted due to the "only instantiation"
logic, right?)
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1471
+ void foo() {
+ bar(1, 2);
+ }
----------------
could reasonably expect this to be `bar(a:1, 2)` or `bar(a:1, a:2)` - leave a
comment explaining why?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130260/new/
https://reviews.llvm.org/D130260
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
