upsj marked an inline comment as done. upsj added a comment. yes, I have commit access
================ Comment at: clang-tools-extra/clangd/InlayHints.cpp:483 + !Type.getNonReferenceType().isConstQualified() && + !isExpandedParameterPack(Param); } ---------------- 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? 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
