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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:255
+  )cpp",
+                       ExpectedHint{"a: ", "param"});
+}
----------------
upsj wrote:
> sammccall wrote:
> > upsj wrote:
> > > I haven't been able to figure out why, but for some reason the `CXXConstructExpr` and `CXXTemporaryObjectExpr` are not being picked up by the `RecursiveASTVisitor`, while in a similar situation below, the `CallExpr` gets picked up by `VisitCallExpr`.
> > The AST here of the instantiated bar looks like
> > ```
> > -FunctionDecl <line:5:1, line:7:1> line:5:4 used bar 'S *(int &&)'
> >   |-TemplateArgument type 'S'
> >   |-TemplateArgument pack
> >   | `-TemplateArgument type 'int'
> >   |-ParmVarDecl <col:8, col:18> col:18 used args 'int &&'
> >   `-CompoundStmt <col:24, line:7:1>
> >     `-ReturnStmt <line:6:3, col:23>
> >       `-CXXNewExpr <col:10, col:23> 'S *' Function 'operator new' 'void *(unsigned long)'
> >         `-CXXConstructExpr <col:14, col:23> 'S':'S' 'void (int)' list
> >           `-ImplicitCastExpr <col:16> 'int':'int' <LValueToRValue>
> >             `-DeclRefExpr <col:16> 'int':'int' lvalue ParmVar 'args' 'int &&'
> > ```
> > 
> > So there's no CXXTemporaryObjectExpr (the value here is a pointer), but should be a CXXConstructExpr. 
> > 
> > Are you sure you're traversing bar's instantiation/specializaion, as opposed to its templated/generic FunctionDecl? The AST of the templated FunctionDecl indeed has an InitListExpr in place of the CXXConstructExpr because it's not resolved yet.
> It's quite possible this is related to the visitor not/only inconsistently traversing template instantiations due to shouldVisitTemplateInstantiations returning false. But I need to implement the new approach first, anyways.
Ah, I think I see the confusion.

We don't want to gather all the information in a single traversal of the whole AST. Such a traversal would need to walk all instantiated templates including those from headers, which is far too much/too slow.

Instead, you want to analyze particular forwarding functions by walking through their bodies looking for the forwarding call. RecursiveASTVisitor is the tool for this job - you just call TraverseDecl(FunctionDecl) to limit the scope. The visitor should not visit template instantiations, but the traversal can still be rooted at an instantiation!

So something roughly like:

```
Optional<vector<ParmVarDecl*>> ultimateParams(FunctionDecl* Callee) {
  if (!Callee || !isa<FunctionProtoType>(Callee->getType()))
    return None;
  if (not instantiated from template || Pattern doesn't use a pack)
    return Callee->parameters();

  for (P : Pattern->parameters()) {
    if (!P->isPack())
      Result.push_back(Callee->parameters[Pos++]);
    else {
      if (PackExpansionSize = ...) {
        if (ForwardedParams = forwardedParams(Callee, P))
          Result.append(*ForwardedParams);
        else // failed, no param info for forwarded args
          Result.append(nullptr, PackExpansionSize);
        Pos += PackExpansionSize;
      }
    }
  }
}

Optional<vector<ParmVarDecl*>> forwardedParams(FunctionDecl *Callee, ParmVarDecl *Pack) {
  class ForwardFinder : RAV<ForwardFinder> {
    bool VisitCallExpr(CallExpr *E) {
      if (E->params() expands Pack) {
        if (CallParams = ultimateParams(E->getCallee()->getAsFunctionDecl()))
          Result = CallParams.slice(section corresponding to Pack);
      }
    }
  };
  ForwardFinder Finder;
  if (Callee->hasBody())
    return Finder.VisitCallExpr(Callee);  
}
```

you could also memoize with a map, but I suspect simple recursion is going to be good enough


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124690