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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
