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

[Tobias Ribizel via Phabricator via cfe-commits]

upsj created this revision.
upsj added a reviewer: nridge.
Herald added subscribers: usaxena95, kadircet, arphaman.
Herald added a project: All.
upsj updated this revision to Diff 426127.
upsj added a comment.
upsj added a reviewer: sammccall.
upsj updated this revision to Diff 426224.
upsj updated this revision to Diff 426225.
upsj published this revision for review.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

remove accidentally committed file


upsj added a comment.

remove dependency on D124359 <https://reviews.llvm.org/D124359> and add tests


upsj added a comment.

remove unnecessary changes



================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:388
+    // arguments
+    auto ForwardedParmMatcher = compoundStmt(forEachDescendant(
+        invocation(
----------------
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.


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:388
+    // arguments
+    auto ForwardedParmMatcher = compoundStmt(forEachDescendant(
+        invocation(
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:255
+  )cpp",
+                       ExpectedHint{"a: ", "param"});
+}
----------------
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`.


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:270
+  )cpp",
+                       ExpectedHint{"a: ", "wrapped"},
+                       ExpectedHint{"a: ", "param"});
----------------
This is a bit of an unfortunate side-effect of looking at instantiated templates.


This adds special-case treatment for parameter packs in make_unique-like functions to forward parameter names to inlay hints.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124690

Files:
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D124690.426225.patch
Type: text/x-patch
Size: 11754 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220430/46f51f51/attachment-0001.bin>