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

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added inline comments.


================
Comment at: clang-tools-extra/clangd/AST.cpp:690
+getPackTemplateParameter(const FunctionDecl *Callee) {
+  if (const auto *TemplateDecl = Callee->getPrimaryTemplate()) {
+    auto TemplateParams = TemplateDecl->getTemplateParameters()->asArray();
----------------
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?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124690