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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Finished looking through the patch; I found it nicely organized and fairly easy to understand (as easy as code involving the analysis of C++ variadic templates can be, anyways :-D)!



================
Comment at: clang-tools-extra/clangd/AST.cpp:682
+      if (const auto *TTPD =
+              dyn_cast<TemplateTypeParmDecl>(TemplateParams.back())) {
+        const auto *TTPT =
----------------
I don't think there is any requirement that a pack be a trailing **template** parameter. For example, the following is valid:

```
template <typename... B, typename A>
void foo(A, B...);

void bar() {
  foo(1, 2, 3);
}
```


================
Comment at: clang-tools-extra/clangd/AST.cpp:761
+  // inspects the given callee with the given args to check whether it
+  // contains Parameters, and sets FullyResolved, PartiallyResolved and
+  // NextTarget accordingly.
----------------
These names (FullyResolved, PartiallyResolved, NextTarget) sound like they might be leftovers from a previous implementation?


================
Comment at: clang-tools-extra/clangd/AST.cpp:831
+  static FunctionDecl *resolveOverload(UnresolvedLookupExpr *Lookup,
+                                       CallExpr *D) {
+    FunctionDecl *MatchingDecl = nullptr;
----------------
nit: `D` is an odd name for a `CallExpr`, maybe `Call` or `E`?


================
Comment at: clang-tools-extra/clangd/AST.cpp:888
+    // Split the parameters into head, pack and tail
+    auto IsExpandedPack = [&](const ParmVarDecl *P) {
+      return getPackTemplateParameter(P) == TTPT;
----------------
can just capture `TTPT` here


================
Comment at: clang-tools-extra/clangd/AST.cpp:899
+    auto HeadIt = std::copy(Head.begin(), Head.end(), Result.begin());
+    auto TailIt = std::copy(Tail.rbegin(), Tail.rbegin(), Result.rbegin());
+    // Recurse on pack parameters
----------------
The second argument is presumably meant to be `Tail.rend()`


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


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:541
 
+    // Remove parameter names that occur multiple times completely.
+    llvm::StringMap<size_t> NameLastSeen;
----------------
This is an interesting approach for handling `VariadicRecursive`.

I had in mind a different approach, something like keeping a `std::set<FunctionTemplateDecl*> SeenFunctionTemplates` in `resolveForwardingParameters()`, populating it with `CurrentFunction->getPrimaryTemplate()` on each iteration, and bailing if the same function template is seen more than once (indicating recursion). But this approach seems to work too, as a parameter name can't legitimately appear twice in a function declaration.

That said, maybe having such a `SeenFunctionTemplates` recursion guard would be helpful anyways, so that e.g. in `VariadicInfinite`, we would bail after a single recursion rather than going until `MaxDepth`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124690