ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
The new test fails in the pre merge checks.
Maybe the diff is missing some more changes?
================
Comment at: clang-tools-extra/clangd/AST.cpp:794
}
auto OptPackLocation = findPack(Args);
+ if (!OptPackLocation)
----------------
NIT: maybe rename to `PackLocation`?
now that there is only 1 variable we could use a shorter name without any
confusion
================
Comment at: clang-tools-extra/clangd/AST.cpp:828
+ if (const auto *RefArg = unwrapForward(Arg)) {
+ if (Parameters.front() != dyn_cast<ParmVarDecl>(RefArg->getDecl()))
+ continue;
----------------
NIT: doesn't this work without `dyn_cast`? looks redundant, I expect
derived-to-base conversion to do the right thing.
================
Comment at: clang-tools-extra/clangd/AST.cpp:871
- // Removes any implicit cast expressions around the given expression.
- static const Expr *unwrapImplicitCast(const Expr *E) {
- while (const auto *Cast = dyn_cast<ImplicitCastExpr>(E)) {
- E = Cast->getSubExpr();
- }
- return E;
- }
-
- // Maps std::forward(E) to E, nullptr otherwise
- static const Expr *unwrapForward(const Expr *E) {
+ // Maps std::forward(E) to E, identity otherwise.
+ static const DeclRefExpr *unwrapForward(const Expr *E) {
----------------
Could you update the comment to account for the new semantics?
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1446
+ void bax(Args... args) { foo({args...}, args...); }
+
+ void foo() {
----------------
NIT: maybe test for the case with a single expansion here:
```
foo(Foo(args...), 1);
foo({args...}, 1);
```
?
testing multiple expansions is also interesting, but seems orthogonal to the
change being made here.
E.g. tests for it would probably benefit from more than 2 appearances of `args`
and more complicated nesting structures.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130261/new/
https://reviews.llvm.org/D130261
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
