upsj added inline comments.
================ Comment at: clang-tools-extra/clangd/InlayHints.cpp:483 + !Type.getNonReferenceType().isConstQualified() && + !isExpandedParameterPack(Param); } ---------------- sammccall wrote: > upsj wrote: > > sammccall wrote: > > > upsj wrote: > > > > sammccall wrote: > > > > > sammccall wrote: > > > > > > nridge wrote: > > > > > > > sammccall wrote: > > > > > > > > why is this check needed if we already decline to provide a > > > > > > > > name for the parameter on line 534 in chooseParameterNames? > > > > > > > `shouldHintName` and `shouldHintReference` are [two independent > > > > > > > conditions](https://searchfox.org/llvm/rev/508eb41d82ca956c30950d9a16b522a29aeeb333/clang-tools-extra/clangd/InlayHints.cpp#411-418) > > > > > > > governing whether we show the parameter name and/or a `&` > > > > > > > indicating pass-by-mutable-ref, respectively > > > > > > > > > > > > > > (I did approve the [patch](https://reviews.llvm.org/D124359) that > > > > > > > introduced `shouldHintReference` myself, hope that's ok) > > > > > > Thanks, that makes sense! I just hadn't understood that change. > > > > > What exactly *is* the motivation for suppressing reference hints in > > > > > the pack case? > > > > > > > > > > (I can imagine there are cases where they're annoying, but it's hard > > > > > to know if the condition is right without knowing what those are) > > > > I added an explanation. Basically, if we are unable to figure out which > > > > parameter the arguments are being forwarded to, the type of the > > > > ParmVarDecl for `Args&&...` gets deduced as `T&` or `T&&`, so that > > > > would mean even though we don't know whether the argument will > > > > eventually be forwarded to a reference parameter, we still claim all > > > > mutable lvalue arguments will be mutated, which IMO introduces more > > > > noise than necessary. But I think there are also good arguments for > > > > adding them to be safe. > > > > > > > > There is another detail here, which is that we don't record whether we > > > > used std::forward, so the corresponding rvalue-to-lvalue conversions > > > > may lead to some unnecessary & annotations for rvalue arguments. > > > This makes sense, the comment explains well, thank you! > > > I have a couple of quibbles, up to you whether to change the logic. > > > > > > #1: There's an unstated assumption that pack arguments *will* be > > > forwarded (there are other things we can do with them, like use them in > > > fold-expressions). It's a pretty good assumption but if the comment talks > > > about forwarding, it should probably mention explicitly ("it's likely the > > > params will be somehow forwarded, and...") > > > > > > #2: the idea is that if the reference-ness is deduced from the callsite, > > > then it's not meaningful as an "is the param modified" signal, it's just > > > "is this arg modifiable". Fair enough, but this is a property of > > > universal/forwarding references (T&& where T is a template param), not of > > > packs. So I *think* this check should rather be > > > !isInstantiatedFromForwardingReference(Param). > > > But maybe that's more complexity and what you have is a good heuristic - > > > I think at least we should call out that it's a heuristic for the true > > > condition. > > > > > > > > #1: I agree, I'll make that more clear before committing. > > > > #2: Now that I think about it, there are actually two things we don't keep > > track of: parameters could lose their reference-ness via `Args...` instead > > of `Args&&...` and their rvalue-ness by not using `std::forward`. We only > > look at whether the innermost call takes a reference parameter, but as I > > said, we may lose some of that information on the way, claiming the > > function may modify the argument when it actually creates a copy on the way > > (losing reference-ness). I think the case of an rvalue being mistaken for > > an lvalue should not be much of an issue, since the reference annotation > > almost makes sense. > > > > To visualize the situation: These three snippets all add &: hints to the > > parameter of bar > > ``` > > void foo(int&); > > template <typename... Args> > > void bar(Args... args) { return foo(args...); } > > void baz() { > > bar(1); > > } > > ``` > > ``` > > void foo(int&); > > template <typename... Args> > > void bar(Args&&... args) { return foo(args...); } > > void baz() { > > bar(1); > > } > > ``` > > ``` > > void foo(int&); > > template <typename... Args> > > void bar(Args&&... args) { return foo(std::forward<Args>(args)...); } > > void baz() { > > int a; > > bar(a); > > } > > ``` > > Two of these three cases probably shouldn't have this annotation? > > parameters could lose their reference-ness via Args... instead of Args&&... > (I'm not quite following what you mean here: if we deduce as `Args` rather > than `Args&&` then the parameters are not references in the first place, > we're passing by value) > > > and their rvalue-ness by not using std::forward > Yes. Fundamentally if we're deducing the ref type then we should be looking > for a concrete signal of how the value is ultimately used, which involves > tracking casts like std::forward. This is true whether it's a pack or not. > It's a bunch of work and I'm not sure it's worth it (and I'm certainly not > asking you to add it!) > > > Two of these three cases probably shouldn't have this annotation? > Yes. Claiming we're passing a numeric literal (prvalue) by mutable reference > doesn't pass the laugh test. > > > We only look at whether the innermost call takes a reference parameter, but > > as I said, we may lose some of that information on the way, claiming the > > function may modify the argument when it actually creates a copy on the way > > (losing reference-ness) > > Right, there can be copies on the way for various reasons (passing by value, > but also e.g. forwarding a T as a const U& where U is constructible from T). > > I'm starting to think the simplest answer for now is never to include `&` if > there's a forwarding T&& reference involved, as it's not always clear what it > means and it's hard to do reliably. > > This means we're accepting that `make_unique<Foo>(RefParamToConstructor)` > will lack its `&`. (It already does today - this patch would need extensions > to do this robustly). > > I think we only need to check the outer call in the usual way, not any inner > forwarded calls: > - if we're passing as (deduced) T then that's by value and no `&` is needed > - if we're passing as (deduced) T& then that's explicitly by reference (will > only bind to a mutable lvalue) and `&` is needed > - if we're passing as (deduced) T&& then we're going to conservatively not > include the `&` > > I don't think packs need to be involved in this logic at all. I //think// I found a suitable proxy for figuring out whether the function uses `Args&&...` and/or `std::forward`, can you take a final look at `shouldHintReference` whether this makes sense to you or I may have missed any important edge cases? I also added a few tests for this behavior (`VariadicReferenceHint...`) that should show that it works. 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
