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