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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:611
 
+  // FIXME: once inlayHints can disabled per-file in config, always advertise.
   if (Opts.InlayHints)
----------------
s/can/can be


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:168
+      addInlayHint(Args[I]->getSourceRange(), HintSide::Left,
+                   InlayHintKind::ParameterHint, "", Name, ": ");
     }
----------------
nit: parameter name comments for `""` and `": "` (same for other calls)


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:320
 
-  void addInlayHint(SourceRange R, InlayHintKind Kind, llvm::StringRef Label) {
+  // We pass HintSide rather than SourceLocation because we want to ensure we
+  // it is in the same file as the common file range.
----------------
drop the last `we`


================
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.
----------------
nit: drop the empty line?


================
Comment at: clang-tools-extra/clangd/Protocol.h:1555
+  ///
+  /// For example, an parameter hint may be positioned before an argument.
+  Position position;
----------------
s/an parameter/a parameter/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116699