[PATCH] D130261: [clangd] Refactor forwarding call detection logic

[Ilya Biryukov via Phabricator via cfe-commits]

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