[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Thanks for the readability improvements! I've forgotten if you have commit access?

This stuff is complicated and I'm definitely going to forget the reasoning, the intuitive explanations are going to help a lot if changes are needed in future.
(Maybe it's just me, but the formal language around instantiations, deductions, etc makes my eyes glaze over - as if it's the words themselves rather than how you use them!)



================
Comment at: clang-tools-extra/clangd/AST.cpp:690
+getPackTemplateParameter(const FunctionDecl *Callee) {
+  if (const auto *TemplateDecl = Callee->getPrimaryTemplate()) {
+    auto TemplateParams = TemplateDecl->getTemplateParameters()->asArray();
----------------
upsj wrote:
> sammccall wrote:
> > This is doing something pretty strange if Callee is a function template specialization.
> > 
> > It's not clear to me whether this function should be handling that case (which AFAICS it doesn't, but could inspect the specialization kind), or whether resolveForwardingParameters is responsible for not calling this function in that case (in which case we should probably have an assert here).
> > 
> > Can you also add a test case that function template specialization doesn't confuse us? i.e. it should return the parmvardecls from the specialization's definition.
> Do the new tests `VariadicNameFromSpecialization(Recursive)?` match what you had in mind?
Yes, that's fantastic, thank you!


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




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690