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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Sorry about the delay. This is complicated stuff and every time I put it down for a day I forget how it works!
Thanks for your hard work here.

I have some more suggestions but hopefully fairly mechanical.
Feel free to land once you're done, or send it back over to Nathan or I to commit.



================
Comment at: clang-tools-extra/clangd/AST.cpp:677
+
+// Checks if the template parameter declaration is a type parameter pack
+bool isTemplateTypeParameterPack(NamedDecl *D) {
----------------
nit: this comment doesn't really say anything that the method name doesn't say already.

Maybe replace with an example like "returns true for `X` in `template <class...X> int m;`"


================
Comment at: clang-tools-extra/clangd/AST.cpp:680
+  if (const auto *TTPD = dyn_cast<TemplateTypeParmDecl>(D)) {
+    const auto *TTPT = TTPD->getTypeForDecl()->castAs<TemplateTypeParmType>();
+    return TTPT->isParameterPack();
----------------
TTPD->isParameterPack() does the same thing with less code


================
Comment at: clang-tools-extra/clangd/AST.cpp:689
+const TemplateTypeParmType *
+getPackTemplateParameter(const FunctionDecl *Callee) {
+  if (const auto *TemplateDecl = Callee->getPrimaryTemplate()) {
----------------
it seems confusing and unneccesary to use the same name for the two versions of this function. The name applies to both, but they don't really do the same thing.

Maybe call this one getFunctionPackType and the other getUnderylingPackType?
(not get*TemplateParameter as they return a type, not the parameter decl)


================
Comment at: clang-tools-extra/clangd/AST.cpp:690
+getPackTemplateParameter(const FunctionDecl *Callee) {
+  if (const auto *TemplateDecl = Callee->getPrimaryTemplate()) {
+    auto TemplateParams = TemplateDecl->getTemplateParameters()->asArray();
----------------
This is doing something pretty strange if Callee is a function template specialization.

It's not clear to me whether this function should be handling that case (which AFAICS it doesn't, but could inspect the specialization kind), or whether resolveForwardingParameters is responsible for not calling this function in that case (in which case we should probably have an assert here).

Can you also add a test case that function template specialization doesn't confuse us? i.e. it should return the parmvardecls from the specialization's definition.


================
Comment at: clang-tools-extra/clangd/AST.cpp:720
+
+class ForwardingCallVisitor
+    : public RecursiveASTVisitor<ForwardingCallVisitor> {
----------------
This class could use a high-level comment explaining what it does.

e.g.
```
This visitor walks over the body of an instantiated function template.
The template accepts a parameter pack and the visitor records whether
the pack parameters were forwarded to another call. For example, given:

template <class T, class...Args>
auto make_unique(Args..args) {
  return unique_ptr<T>(new T(args...));
}

When called as `make_unique<std::string>(2, 'x')` this yields a function
`make_unique<std::string, int, char>` with two parameters.
The visitor records that those two parameters are forwarded to the
`constructor std::string(int, char);`.

This information is recorded in the `ForwardingInfo` (in general,
more complicated scenarios are also possible).
```


================
Comment at: clang-tools-extra/clangd/AST.h:212
+/// reference to one (e.g. `Args&...` or `Args&&...`).
+bool isExpandedParameterPack(const ParmVarDecl *D);
+
----------------
nit: I think isExpanded**From**ParameterPack might be clearer, up to you


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:398
+    auto Params = resolveForwardingParameters(Callee);
+    // We are only interested in expanded arguments with corresponding
+    // parameters.
----------------
I can't understand what this comment is saying, can you rephrase it or provide an example?

>From looking at the code, I'm guessing something like:
"If we're passing a parameter pack into this call, we need to give up matching arguments to parameters at that point as we don't know how long it is".

---

I think the interaction between getExpandedArgCount and chooseParameterNames is unclear and brittle here. (i.e. the invariant that `getExpandedArgCount(Args).size() < chooseParameterNames(Params).size()` is very non-local)

I'd suggest writing this more directly as:
```
for (I = 0; I < ParameterNames.size() && I < Args.size(); ++I) {
  // ... explanation ...
  if (isa<PackExpansionExpr>(Args[I]))
    break;

  // ... generate param hint ...
}
```


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:483
+           !Type.getNonReferenceType().isConstQualified() &&
+           !isExpandedParameterPack(Param);
   }
----------------
why is this check needed if we already decline to provide a name for the parameter on line 534 in chooseParameterNames?


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:541
+        if (SimpleName.empty()) {
+          if (auto *Callee = dyn_cast<FunctionDecl>(P->getDeclContext())) {
+            if (auto *Def = Callee->getDefinition()) {
----------------
factor out a function getParamDefinition?


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:568
 
-  // Return the number of fixed parameters Function has, that is, not counting
-  // parameters that are variadic (instantiated from a parameter pack) or
-  // C-style varargs.
-  static size_t getFixedParamCount(const FunctionDecl *Function) {
-    if (FunctionTemplateDecl *Template = Function->getPrimaryTemplate()) {
-      FunctionDecl *F = Template->getTemplatedDecl();
-      size_t Result = 0;
-      for (ParmVarDecl *Parm : F->parameters()) {
-        if (Parm->isParameterPack()) {
-          break;
-        }
-        ++Result;
-      }
-      return Result;
-    }
-    // C-style varargs don't need special handling, they're already
-    // not included in getNumParams().
-    return Function->getNumParams();
+  // Return the length of the prefix of arguments that are not unexpanded pack
+  // expansion expressions.
----------------
the name of this function is very confusing - even after understanding what it does, I can't understand how that corresponds to the name.

However as noted above I don't think we need this function returning an index at all, instead we can just check the condition while looping.


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:573
+        Args.begin(), std::find_if(Args.begin(), Args.end(), [](const Expr *E) {
+          return dyn_cast<PackExpansionExpr>(E) != nullptr;
+        }));
----------------
isa<PackExpansionExpr>(E)


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:515
+      if (isExpandedParameterPack(P)) {
+        ParameterNames.emplace_back();
+      } else {
----------------
nridge wrote:
> let's add `// will not be hinted` for clarity
say why, not what

```
// If we haven't resolved a pack paramater (e.g. foo(Args... args)) then
// hinting as foo(args: 1, args: 2, args: 3) is unlikely to be useful.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124690