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