[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

This revision was automatically updated to reflect the committed changes. Closed by commit rG9e6a342fdac8: [clangd] Implement end-definition-comment inlay hints (authored by daiyousei-qz, committed by sammccall). Changed prior to commit: https://reviews.llvm.org/D150635?vs=525922&id=540750#toc

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

sammccall added a comment. Oh no, I just saw this never landed - I suspect you were waiting for me to commit it, and I was expecting you to do it! I've rebased and I'll commit it for you now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150635/ne

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz marked an inline comment as done. daiyousei-qz added a comment. Addressed the remaining comments. Thanks everyone for helping review and improve this patch! Comment at: clang-tools-extra/clangd/InlayHints.cpp:802 +Position HintStart = sourceLocToPosition(SM, R

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz updated this revision to Diff 525922. daiyousei-qz marked 8 inline comments as done. daiyousei-qz added a comment. - Address remaining comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150635/new/ https://reviews.llvm.org/D150635

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. > 150ms is more than noticeable Fair enough. I also see we *are* avoiding the quadratic algorithm in other places in inlay hints, and that we can't easily reuse the getHintRange() funct

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz added inline comments. Comment at: clang-tools-extra/clangd/InlayHints.cpp:578 +constexpr unsigned HintMinLineLimit = 2; +constexpr unsigned HintMaxLengthLimit = 50; + nridge wrote: > daiyousei-qz wrote: > > sammccall wrote: > > > We actually

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

nridge added inline comments. Comment at: clang-tools-extra/clangd/InlayHints.cpp:578 +constexpr unsigned HintMinLineLimit = 2; +constexpr unsigned HintMaxLengthLimit = 50; + daiyousei-qz wrote: > sammccall wrote: > > We actually already have a configurab

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz added a comment. Addressed review comments except for those we don't have an agreement yet. Comment at: clang-tools-extra/clangd/InlayHints.cpp:578 +constexpr unsigned HintMinLineLimit = 2; +constexpr unsigned HintMaxLengthLimit = 50; + sam

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz updated this revision to Diff 525418. daiyousei-qz marked 9 inline comments as done. daiyousei-qz added a comment. - Move computeBlockEndHintRange - Correct label length limit logic - Correct line number computation for '{' - Address other review comments Repository: rG LLVM Githu

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz added a comment. Yeah, makes sense. Thank you for your insight! I missed the fact that this time include AST build time. I manually injected a time region into `inlayHints`, here's part of logs printed for scrolling "offsetToPosition" build: I[23:36:43.055] <-- textDocument/hove

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

sammccall added a comment. In D150635#4366730 , @daiyousei-qz wrote: > I'm not sure but 18ms is definitely wrong here, especially for a debug build. > In your log, even semantic highlighting builds in 60ms, which looks > incorrect. I tried again with T

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz added a comment. I'm not sure but 18ms is definitely wrong here, especially for a debug build. In your log, even semantic highlighting builds in 60ms, which looks incorrect. I tried again with TOT clangd and noticed the following pattern. - If I scroll with vscode, inlay hints typi

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

sammccall added inline comments. Comment at: clang-tools-extra/clangd/InlayHints.cpp:616 +Position HintStart = sourceLocToPosition(SM, BraceRange.getEnd()); +Position HintEnd = {HintStart.line, +HintStart.character + daiyousei-qz w

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

sammccall added a comment. Thanks, I'll try to repro those results. 100ms is significant in absolute terms, but >1s seems unacceptably slow. I believe VSCode always sends ranges along with latency-sensitive hint requests, I think we currently just post-filter. If we propagate the limits deeper w

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz added inline comments. Comment at: clang-tools-extra/clangd/InlayHints.cpp:616 +Position HintStart = sourceLocToPosition(SM, BraceRange.getEnd()); +Position HintEnd = {HintStart.line, +HintStart.character + sammccall w

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

sammccall added a comment. > Though I'm proposing a name like "DeclBlockEnd" to make it clearer I prefer the current name. DeclBlockEnd is both too long and contains a cryptic abbreviation (should be DeclarationBlockEnd). And I don't think distinguishing between declaration blocks and flow-cont

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz marked an inline comment as done. daiyousei-qz added a comment. Addressed most review comments. I'm currently using the name "BlockEnd" as suggested. Though I'm proposing a name like "DeclBlockEnd" to make it clearer. What do you think? Comment at: clang-tools-ex

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz updated this revision to Diff 524183. daiyousei-qz marked 19 inline comments as done. daiyousei-qz added a comment. Address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150635/new/ https://reviews.llvm.org/D150635 Fil

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

sammccall added a comment. In D150635#4348702 , @daiyousei-qz wrote: > Regarding to **Naming**, I agree that `EndDefinitionComment` sounds so > tedious but I couldn't come up with a better name. However, I think > `EndBlock` also feel a little strange

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

zyounan added a comment. Thank you for the update. A few questions: - Can you add a test with macro expansion? Would we get two inlay hints right after the spelling location? e.g., #define Decl_Record \ struct Widget { \ [very long definition that reach `EndDefinitionCommentMinLine

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz added a comment. For a quick peek to the hint: F27483170: image.png Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150635/new/ https://reviews.llvm.org/D150635 _

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz added a comment. @zyounan np! Feel free to leave your review comments! @sammccall Thank you for you insightful and detailed review! I addressed many review comments in the updated patch and left things I'm not very sure. Regarding to **Naming**, I agree that `EndDefinitionComment`

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz updated this revision to Diff 522898. daiyousei-qz marked 16 inline comments as done. daiyousei-qz added a comment. - addressed many review comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150635/new/ https://reviews.llvm.org/D150

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

sammccall added inline comments. Comment at: clang-tools-extra/clangd/InlayHints.cpp:292 + + bool VisitRecordDecl(RecordDecl *D) { +if (Cfg.InlayHints.EndDefinitionComments && Not sure why if need to handle this + EnumDecl, can we just handle VisitTagDecl t

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

sammccall added a comment. Thanks for doing this! Some high-level comments here, will review implementation next: **There seems to be a lot of "decoration" on the hint text.** If declaring a class "Foo" then we add ` /* class ` before and ` */ ` after Foo, for a total of 14 characters apart fr

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

zyounan added a comment. Sorry for chiming in. Left a few nit comments and I hope you don't mind. :) Comment at: clang-tools-extra/clangd/InlayHints.cpp:772 + Label = printName(AST, D); +} else { + // We handle type and namespace decls together. Q

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz added a comment. Requesting for some advice Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1666 + +$anon[[struct { + int x; This is unwanted behavior from my understanding. Do you guys have any insight how we could fix

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz updated this revision to Diff 522434. daiyousei-qz added a comment. minor naming fix2 (last fix breaks builds) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150635/new/ https://reviews.llvm.org/D150635 Files: clang-tools-extra/clang

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz updated this revision to Diff 522432. daiyousei-qz edited the summary of this revision. daiyousei-qz added a comment. minor naming fix Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150635/new/ https://reviews.llvm.org/D150635 Files:

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

daiyousei-qz created this revision. Herald added subscribers: kadircet, arphaman. Herald added a project: All. daiyousei-qz requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-extra. Repository: rG LLVM Github Mono