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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Will continue looking at the implementation tomorrow, for now a few minor comments and a suggested variation on a testcase.



================
Comment at: clang-tools-extra/clangd/AST.h:19
 #include "clang/AST/DeclObjC.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/NestedNameSpecifier.h"
----------------
nit: this include seems unnecessary given the code being added


================
Comment at: clang-tools-extra/clangd/AST.h:210
+
+/// Checks whether D is the expanded form of a parameter pack
+bool isExpandedParameterPack(const ParmVarDecl *D);
----------------
This function does something a bit more specific than what this description suggests: given a type parameter pack `typename... T`, it will return true for function parameters instantiated from a parameter pack of type `T...` or `T&...` or `T&&...`, but not `T*...` or `vector<T>...` or anything else.

Suggested description:

```
/// Checks whether D is instantiated from a function parameter pack
/// whose type is a bare type parameter pack (e.g. `Args...`), or a 
/// reference to one (e.g. `Args&...` or `Args&&...`).
```


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:200
+  )cpp",
+                       ExpectedHint{"&: ", "fwd"});
+}
----------------
As an aside, `&` does not seem like a useful hint to show for `std::forward` -- should we try to omit it? (We don't need to do it in this patch as it's not really related.)


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:467
+
+TEST(ParameterHints, VariadicVarargs) {
+  assertParameterHints(R"cpp(
----------------
I think a variation of this test case where `foo` is also variadic, would be valuable to add:

```
    template <typename... Args>
    void foo(int fixed, Args&&... args);

    template <typename... Args>
    void bar(Args&&... args) {
      foo(args...);
    }

    void baz() { 
      bar($fixed[[41]], 42, 43);
    }
```

This case does seem to already work with the current patch, and I think it's the more common case; I'm happy to defer the varargs one as a less important edge case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124690