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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:388
+    // arguments
+    auto ForwardedParmMatcher = compoundStmt(forEachDescendant(
+        invocation(
----------------
upsj wrote:
> sammccall wrote:
> > We tend not to use ASTMatchers for these kind of tasks in clangd (except when embedding clang-tidy checks of course).
> > 
> > I guess the biggest technical reason is it's hard to reason about their performance so they end up slow and opaque (and indeed the clang-tidy checks are slow and we haven't got a clear idea how to fix it). It's also easy to match too much.
> > 
> > But part of it is just convention - because we have more RecursiveASTVisitors, the maintainers are more familiar with the quirks there than the quirks of matchers.
> > 
> > ---
> > 
> > To be clear, I don't see any specific problems with this code! But we still might ask to change it as there are costs to mixing styles, and we don't want to end up in a situation where a bug fix requires choosing between an expensive hasAncestor() matcher and rewriting the logic.
> That makes sense. From the Visitor standpoint, do I understand correctly that you mean something like remembering the top-level templated `FunctionDecl` (or stack of `FunctionDecl`s if we have something like nested Lambdas?) and check every `CallExpr` and `CXXConstructExpr` for `ParmVarDecls` or `std::forward(ParmVarDecl)` matching the parameters of the higher-level `FunctionDecls`? That means basically lazily evaluating the parameter names, so more storage inside the Visitor, but allows us to do recursive resolution in a rather straightforward fashion.
I imagine something like running a `RecursiveASTVisitor` on `Callee->getBody()`, and overriding `VisitCallExpr()` and `VisitCXXConstructExpr()` to check whether it is a call of the right form.

I don't //think// theres a need to worry about lambdas nested in the body, I think we may still be able to get useful parameter names out of them, as in e.g.:

```
struct S {
 S(int a, int b);
};

template <typename T, typename... Args>
auto Make(Args... args) {
  auto ConstructTask = [&](){ return T(args...); };
  return ConstructTask();
}

int main() {
  auto s = Make<S>(1, 2);
}
```


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:474
+    // the parameter names from the wrapped function
+    if (Args.size() > FixedParamCount && Args.size() == Params.size()) {
+      auto ForwardedParams = matchForwardedParams(
----------------
What is the reason for the `Args.size() == Params.size()` condition?

Doesn't this break cases where there is more than one argument matching the variadic parameter? For example:

```
void foo(int a, int b);
template <typename... Args>
void bar(Args&&... args) { foo(args...); }
int main() {
  bar(1, 2);
}
```

Here, I expect we'd have `Args.size() == 2` and `Params.size() == 1`.

(We should probably have test coverage for such multiple-arguments cases as well. We probably don't need it for all combinations, but we should have at least one test case exercising it.)


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:178
+  // No hint for anonymous variadic parameter
+  // The prototype is not correct, but is converted into builtin anyways.
+  assertParameterHints(R"cpp(
----------------
What does "converted into builtin" mean here?


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:203
+TEST(ParameterHints, VariadicForwardedConstructor) {
+  // No hint for anonymous variadic parameter
+  // The prototype is not correct, but is converted into builtin anyways.
----------------
This comment seems out of sync with the testcase (same for a few others)


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:209
+    template <typename T, typename... Args>
+    T bar(Args&&... args) { return T{$wrapped[[std::forward<Args>(args)...]]}; }
+    void baz() {
----------------
The `wrapped` range doesn't seem to be used anywhere (same for a few other testcases)


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:270
+  )cpp",
+                       ExpectedHint{"a: ", "wrapped"},
+                       ExpectedHint{"a: ", "param"});
----------------
upsj wrote:
> This is a bit of an unfortunate side-effect of looking at instantiated templates.
Perhaps it would make sense to exclude arguments which are pack expansion expressions from getting parameter hints?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124690