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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
