[PATCH] D124344: [clangd] Output inlay hints with `clangd --check`

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

can you please upload the patch with full context? see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface



================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:196
+  // Build Inlay Hints for the entire AST or the specified range
+  bool buildInlayHints(llvm::Optional<Range> LineRange) {
+    log("Building inlay hints");
----------------
this function always returns true, let's make it void


================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:222
 
-      if (!ShouldCheckLine(Pos))
+      if (LineRange && LineRange->contains(Pos))
         continue;
----------------
this should be `LineRange && !LineRange->contains`


================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:296
   if (!C.buildCommand(TFS) || !C.buildInvocation(TFS, Contents) ||
-      !C.buildAST())
+      !C.buildAST() || !C.buildInlayHints(LineRange))
     return false;
----------------
failing inlay hints should not fail the rest, can you put it after `C.testLocationFatures` call below instead?


================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:201
+    for (const auto &Hint : Hints) {
+      vlog("  {0} {1}", Hint.position, Hint.label);
+    }
----------------
nridge wrote:
> upsj wrote:
> > nridge wrote:
> > > Might be useful for print the hint kind as well?
> > right, is the current solution (adding a public toString) okay?
> Looks reasonable to me.
we usually override the `llvm::raw_ostream operator<<` instead, can you do that for `InlayHint` and log `Hint` directly here?


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

https://reviews.llvm.org/D124344