[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Had a read through this. I'm still digesting it, but the high-level approach seems reasonable to me.

Could we add a test case for the recursive scenario that came up during chat:

  void foo();
  
  template <typename Head, typename... Tail>
  void foo(Head head, Tail... tail) {
    foo(tail...);
  }
  
  int main() {
    foo(1, 2, 3);
  }
  `

(even if the behaviour on this testcase isn't what we want yet, it's useful to have it in the test suite as a reminder)



================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:344
+
+  const DeclRefExpr *unpackArgument(const Expr *E) {
+    E = unpackImplicitCast(E);
----------------
unwrap? "unpack" sounds like something related to parameter packs


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:374
+  // have one in the definition, store this mapping here.
+  llvm::DenseMap<const ParmVarDecl *, const ParmVarDecl *> ParamDeclToDef;
+};
----------------
I think it would be more readable if the state that persists across calls (TraversedFunctions, ForwardedParams, ParamDeclToDef) would be separate from the state that does not (UnresolvedParams, TraversalQueue).

A concrete suggestion:

 * Factor the first set out into a `ParameterMappingCache` struct
 * Do not keep a `ForwardingParameterVisitor` as a member of `InlayHintVisitor`, only the `ParameterMappingCache`
 * Create the `ForwardingParameterVisitor` as a local for each call, and pass it a reference to the `ParameterMappingCache` in the constructor


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124690