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

Kadir Cetinkaya via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Apr 25 00:48:25 PDT 2022

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))
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?



More information about the cfe-commits mailing list