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