[PATCH] D116699: [clangd] Polish clangd/inlayHints and expose them by default.

Fri Jan 7 06:08:26 PST 2022

sammccall marked 5 inline comments as done.
sammccall added a comment.

In D116699#3227203 <https://reviews.llvm.org/D116699#3227203>, @kadircet wrote:

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

================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:168
+      addInlayHint(Args[I]->getSourceRange(), HintSide::Left,
+                   InlayHintKind::ParameterHint, "", Name, ": ");
     }
----------------
kadircet wrote:
> nit: parameter name comments for `""` and `": "` (same for other calls)
We have inlay hints now, who needs comments?!

================
Comment at: clang-tools-extra/clangd/InlayHints.h:26
+/// Compute and return inlay hints for a file.
+///
+/// If RestrictRange is set, return only hints whose location is in that range.
----------------
kadircet wrote:
> nit: drop the empty line?
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)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116699