Page MenuHomePhabricator

[clangd] Add inlay hints for mutable reference parameters
ClosedPublic

Authored by upsj on Apr 25 2022, 12:17 AM.

Details

Summary

Add a & prefix to all parameter inlay hints that refer to a non-const l-value reference. That makes it easier to identify them even if semantic highlighting is not used (where this is already available)

Diff Detail

Event Timeline

upsj created this revision.Apr 25 2022, 12:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2022, 12:17 AM
upsj requested review of this revision.Apr 25 2022, 12:17 AM
nridge added a comment.EditedApr 26 2022, 11:34 PM

Thanks for working on this!

Could you add some test cases to InlayHintTests.cpp to illustrate the new hint please?

As in the other patch, uploading the patch with context would also be helpful for review.

upsj updated this revision to Diff 425432.Apr 27 2022, 12:42 AM

add tests for reference inlay hints

upsj added inline comments.Apr 29 2022, 1:38 PM
clang-tools-extra/clangd/InlayHints.cpp
444–446

It probably doesn't make too much sense to highlight r-value references here, since it should be much clearer from context that they are either temporaries or explicitly std::move'd

upsj updated this revision to Diff 426217.Apr 30 2022, 4:19 AM

don't add reference inlay hints for r-value refs

nridge requested changes to this revision.May 1 2022, 11:16 PM

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.

clang-tools-extra/clangd/InlayHints.cpp
444

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).

450

nit: shouldHintName would make more sense to me as a revised method name

clang-tools-extra/clangd/unittests/InlayHintTests.cpp
217

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: .

408

Similarly, and for consistency with other parameter hints, &: would make sense to me here.

This revision now requires changes to proceed.May 1 2022, 11:16 PM
upsj updated this revision to Diff 426343.May 2 2022, 12:30 AM
upsj marked 4 inline comments as done.

address review comments: Move reference hint to the prefix, test type aliases

upsj added a comment.May 2 2022, 12:31 AM

Thanks, on second thought moving the hint to the prefix also makes more sense to me.

Thanks, the changes here look good.

Something is strange about the diff though, it's not letting me see the complete changes relative to the baseline, only the incremental changes relative to a previous version (I think this is also why Phabricator's build status says patch application failed)

clang-tools-extra/clangd/unittests/InlayHintTests.cpp
202

For good measure, let's add an alias = const int& case (expecting no &) as well

upsj updated this revision to Diff 426344.May 2 2022, 12:45 AM

right, arcanist doesn't entirely match my git mental model. Here's the (hopefully) complete patch

upsj updated this revision to Diff 426346.May 2 2022, 12:53 AM
upsj marked an inline comment as done.

add test for const ref inlay hint via type alias

nridge accepted this revision.May 2 2022, 2:29 AM

Thanks!

Final nit: please update the commit message as it's a bit out of date, and then I'll go ahead and merge

clang-tools-extra/clangd/unittests/InlayHintTests.cpp
168

nit: the annotation can be removed

This revision is now accepted and ready to land.May 2 2022, 2:29 AM
upsj edited the summary of this revision. (Show Details)May 2 2022, 2:44 AM
upsj updated this revision to Diff 426372.May 2 2022, 3:54 AM
upsj marked an inline comment as done.

remove unnecessary annotation

This revision was automatically updated to reflect the committed changes.