sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:478
bool shouldHintReference(const ParmVarDecl *Param) {
- // If the parameter is a non-const reference type, print an inlay hint
+ // If the parameter is of non-const l-value reference type not originating
+ // from an unresolved expanded pack, print an inlay hint
----------------
The new condition is nonobvious, but the new comment just echoes the code.
Instead add a second sentence explaining *why*.
(I can't provide a suggestion, because I don't understand why)
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:483
+ !Type.getNonReferenceType().isConstQualified() &&
+ !isExpandedParameterPack(Param);
}
----------------
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.
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:483
+ !Type.getNonReferenceType().isConstQualified() &&
+ !isExpandedParameterPack(Param);
}
----------------
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)
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
