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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
