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