[PATCH] D124359: [clangd] Add inlay hints for mutable reference parameters

[Nathan Ridge via Phabricator via cfe-commits]

nridge requested changes to this revision.
nridge added a comment.
This revision now requires changes to proceed.

Apologies for the slow response here. This generally looks good to me, but I have a couple of suggestions regarding the formatting of the hints and handling type aliases.



================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:448
+    auto Type = Param->getType();
+    return Type->isLValueReferenceType() &&
+                   !Type.getNonReferenceType().isConstQualified()
----------------
We should make sure we handle the case where a parameter is a typedef to a reference type, e.g.:

```
using IntRef = int&;
void foo(IntRef);
int main() {
  int arg;
  foo(arg);  // should show `&: ` hint
}
```

Let's add the testcase for this, and if necessary adjust the code to handle it (it's not immediately obvious to me whether it already handles it as written).


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:454
+
+  bool shouldNameHint(const Expr *Arg, StringRef ParamName) {
     if (ParamName.empty())
----------------
nit: `shouldHintName` would make more sense to me as a revised method name


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:217
+  )cpp",
+                       ExpectedHint{"param: &", "param"});
+}
----------------
I prefer for the hint to continue to end with a space, to create visual separation between the hint and the actual code.

Especially in a case like this where a hint ends in ` &`, it may be difficult to visually distinguish this from the `&` occurring in the actual code.

To that end, I think the hint would make more sense as `param&: ` or `&param: `.


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:382
+  )cpp",
+                       ExpectedHint{"&", "param"});
+}
----------------
Similarly, and for consistency with other parameter hints, `&: ` would make sense to me here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124359