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