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

[Sam McCall via Phabricator via cfe-commits]

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