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

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added a comment.

Yes, I think that's a good summary. Only a small clarification:

> we could query separately where each arg is forwarded to. For multiple forwards, the recursive step is "what param is arg N of this callee ultimately bound to"

I was thinking of implementing this as "what parameters are the following arguments bound to in a direct call", which doesn't have the aforementioned efficiency issues, and is basically a single recursion level of what I had previously implemented. Since (at least afaik) there is no way to have a parameter pack split up between different function calls (at least from the AST standpoint where we here only allow std::forward or plain parameters), the input would be a FunctionDecl and vector/set of ParmVarDecl and the output would be a map ParmVarDecl -> ParmVarDecl plus an Optional<FunctionDecl> telling us whether we need to recurse further. Unrolling the recursion would also make it easier to limit the depth to which we unpack forwarded parameters.



================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:261
+private:
+  void handleParmVarDeclName(const FunctionDecl *Callee, size_t I) {
+    const auto *Param = Callee->getParamDecl(I);
----------------
sammccall wrote:
> upsj wrote:
> > sammccall wrote:
> > > Unless I'm missing something, going looking in the redecls of the function for a parameter name doesn't seem in scope for this patch.
> > > 
> > > We don't support it in inlay hints elsewhere, and it's not clear it has anything to do with forwarding functions.
> > > Maybe the added complexity is justifiable if this logic can be shared with different functions (hover, signature help) but I don't think it belongs in this patch.
> > This was already functionality previously available in `chooseParameterNames`, I thought I would need to do the same thing here, but turns out that I can get the `FunctionDecl` from a `ParmVarDecl`, so this can stay in its previous place.
> You're right, sorry!
> I think the the version we have is pretty opportunistic: this is a couple of lines of code, so why not?
> I don't think we should aim for parity with it, but rather just handle whatever cases are both useful & trivial to implement (in this patch)
> 
> > I can get the FunctionDecl from a ParmVarDecl, so this can stay in its previous place.
> This sounds good to me - means we can drop this map right? That was the main piece I was concerned with.
Yes, we can do the same in the InlayHintVisitor, though it becomes a bit more complex there, since we don't have the index of the argument, which might require a linear search, as different forwarded ParmVarDecls can come from different FunctionDecls


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124690