This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Polish clangd/inlayHints and expose them by default.
ClosedPublic

Authored by sammccall on Jan 5 2022, 1:41 PM.

Details

Summary

This means it's a "real feature" in clangd 14, albeit one that requires special
client support.

  • remove "preview" from the flag description
  • expose the clangdInlayHints capability by default
  • provide position as well as range
  • support InlayHintsParams.range to restrict the range retrieved
  • inlay hint list is in document order (sorted by position)

Still to come: control feature via config rather than flag.

Fixes https://github.com/clangd/clangd/issues/313
Protocol doc is in https://github.com/llvm/clangd-www/pull/56/files

Diff Detail

Event Timeline

sammccall created this revision.Jan 5 2022, 1:41 PM
sammccall requested review of this revision.Jan 5 2022, 1:41 PM
kadircet accepted this revision.Jan 7 2022, 4:57 AM

thanks this LGTM just a couple nits.
my biggest concern is old endpoint will vanish all of a sudden and it might create some confusion, especially for editors that turn on inlayhints without an explicit user interaction.
but i suppose plugin maintainers that implement the extension shouldn't have a hard time finding out the new endpoint and migrating to it. (and I don't think we've got (m)any plugins implementing the extension today.) So all should be fine.

clang-tools-extra/clangd/ClangdLSPServer.cpp
611

s/can/can be

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

nit: parameter name comments for "" and ": " (same for other calls)

321–325

drop the last we

clang-tools-extra/clangd/InlayHints.h
26

nit: drop the empty line?

clang-tools-extra/clangd/Protocol.h
1555

s/an parameter/a parameter/

This revision is now accepted and ready to land.Jan 7 2022, 4:57 AM
sammccall marked 5 inline comments as done.Jan 7 2022, 6:08 AM

thanks this LGTM just a couple nits.
my biggest concern is old endpoint will vanish all of a sudden and it might create some confusion, especially for editors that turn on inlayhints without an explicit user interaction.
but i suppose plugin maintainers that implement the extension shouldn't have a hard time finding out the new endpoint and migrating to it. (and I don't think we've got (m)any plugins implementing the extension today.) So all should be fine.

Yes, transition pains.

To be clear, *this* change isn't breaking anyone: the protocol changes in this patch are backwards-compatible. So the number of plugins implementing today isn't super-relevant.
But we're adding a new API, and encouraging people to use it, and we'll remove it in future. The silver lining: we can choose how long to support the two in parallel, I'd be surprised if it was a significant burden (I expect *much* less than semantic highlights, which is a complicated protocol).

Anyway I don't want to gloss over the bumpiness, it's a tradeoff to get this in front of users.

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

We have inlay hints now, who needs comments?!

clang-tools-extra/clangd/InlayHints.h
26

Done.
(FWIW I find comments a bit easier to read formatted like git commits: one line summary that stands alone, and a blank line before details. But this certainly isn't something we do consistently)

This revision was automatically updated to reflect the committed changes.
sammccall marked 2 inline comments as done.