sammccall added inline comments.

================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:483
+           !Type.getNonReferenceType().isConstQualified() &&
+           !isExpandedParameterPack(Param);
   }
----------------
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.


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

Reply via email to