[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs
Nathan Ridge via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon May 2 00:11:11 PDT 2022
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
More information about the cfe-commits
mailing list