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