This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Output inlay hints with `clangd --check`
ClosedPublic

Authored by upsj on Apr 24 2022, 7:06 AM.

Details

Summary

With the addition of inlay hints to clangd, it would be useful to output them during verbose clangd --check.
This patch adds an output step for inlay hints and unifies the way --check-lines are passed around

Diff Detail

Event Timeline

upsj created this revision.Apr 24 2022, 7:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2022, 7:06 AM
upsj requested review of this revision.Apr 24 2022, 7:06 AM
upsj added a reviewer: nridge.Apr 24 2022, 7:18 AM
nridge accepted this revision.Apr 24 2022, 10:07 PM

Thanks, this seems useful.

clang-tools-extra/clangd/tool/Check.cpp
195

The comment should probably say "... for the entire AST or the specified range"

201

Might be useful for print the hint kind as well?

This revision is now accepted and ready to land.Apr 24 2022, 10:07 PM
upsj updated this revision to Diff 424816.Apr 24 2022, 10:53 PM

Output InlayHintKind and improve comments

upsj updated this revision to Diff 424817.Apr 24 2022, 10:58 PM

forgot to include the old changes in the diff

upsj marked 2 inline comments as done.Apr 24 2022, 10:59 PM
upsj added inline comments.
clang-tools-extra/clangd/tool/Check.cpp
195

done

201

right, is the current solution (adding a public toString) okay?

nridge added inline comments.Apr 25 2022, 12:21 AM
clang-tools-extra/clangd/tool/Check.cpp
201

Looks reasonable to me.

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

clang-tools-extra/clangd/tool/Check.cpp
196

this function always returns true, let's make it void

201

we usually override the llvm::raw_ostream operator<< instead, can you do that for InlayHint and log Hint directly here?

222

this should be LineRange && !LineRange->contains

296

failing inlay hints should not fail the rest, can you put it after C.testLocationFatures call below instead?

upsj updated this revision to Diff 424836.Apr 25 2022, 12:51 AM
upsj marked 2 inline comments as done.
  • format
upsj added a child revision: Restricted Differential Revision.Apr 25 2022, 1:08 AM
upsj updated this revision to Diff 424838.Apr 25 2022, 1:15 AM

Review updates

  • Fix inverted check-lines condition
  • Output InlayHintKind via ostream operators
upsj marked 5 inline comments as done.Apr 25 2022, 1:16 AM
upsj updated this revision to Diff 424839.Apr 25 2022, 1:18 AM

push full patch

(LGTM too, thanks!)

clang-tools-extra/clangd/Protocol.cpp
1322

nit: static (this doesn't need to be public)
nit: return llvm::StringLiteral (which includes the length)

upsj updated this revision to Diff 424846.Apr 25 2022, 1:51 AM
  • use string literal for toString result
upsj marked an inline comment as done.Apr 25 2022, 1:51 AM

Let me know if you'd like me to commit the patch for you.

upsj added a comment.Apr 26 2022, 11:33 PM

@nridge Please do, I don't have commit permissions :)

This revision was automatically updated to reflect the committed changes.