[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge updated this revision to Diff 227374. nridge marked 3 inline comments as done. nridge added a comment. Addressed some nits and got existing tests to pass Will follow up with new tests in the next update Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:469 + auto &AddedLine = DiffedLines.back(); + for (auto Iter = AddedLine.Tokens.begin(); + Iter != AddedLine.Tokens.end();) { hokein wrote: > it took m

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

hokein added a comment. Thanks, I'm fine with the current approach, feel free to add unittests. Comment at: clang-tools-extra/clangd/Protocol.h:1212 std::string Tokens; + /// Is the line in an inactive preprocessor branch? + bool IsInactive = false; could

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge updated this revision to Diff 226563. nridge added a comment. Addressed nits Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67536/new/ https://reviews.llvm.org/D67536 Files: clang-tools-extra/clangd/ParsedAST.cpp clang-tools-extra/clangd

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

ilya-biryukov added a comment. Just a few NITs from my side. I'll let the other review the actual implementation Comment at: clang-tools-extra/clangd/ParsedAST.cpp:217 + void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override { +SkippedRanges.push_ba

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge added a comment. Review / feedback ping :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67536/new/ https://reviews.llvm.org/D67536 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https:

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge added a comment. Ok, I updated the patch to convey the line highlight separately from the token highlights. I did use the simplified approach I mentioned, where I use a single `isInactive` flag. If you'd prefer the more general approach where we send an array of line styles, I can do th

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge updated this revision to Diff 224480. nridge added a comment. Updated to use as isInactive flag on SemanticHighlightingInformation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67536/new/ https://reviews.llvm.org/D67536 Files: clang-tools

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge added a comment. In D67536#1701038 , @ilya-biryukov wrote: > In D67536#1700872 , @nridge wrote: > > > > It also lets them consistently highlight part of the line (e.g. dead > > > expressions or statements c

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

ilya-biryukov added a comment. Just noticed the next version of LSP added diagnostic tags for things like "unused field" or "dead code": https://microsoft.github.io/language-server-protocol/specifications/specification-3-15, search for `DiagnosticTag`. So I guess we won't need ever add range-ba

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

ilya-biryukov added a comment. In D67536#1700872 , @nridge wrote: > > It also lets them consistently highlight part of the line (e.g. dead > > expressions or statements can be marked in gray even if they are on the > > same line). > > Highlighting part o

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge added a comment. In D67536#1699221 , @ilya-biryukov wrote: > I have actually seen clients that just make the text gray and it looks pretty > nice (ReSharper for Visual Studio and IntelliJ IDEA definitely do that). I don't think there is a confli

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge planned changes to this revision. nridge marked an inline comment as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:152 + // Don't bother computing the offset for the end of the line, just use + // zero.

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

ilya-biryukov added a comment. In D67536#1697825 , @sammccall wrote: > So I don't think clients will/should prefer that - for best rendering they > should know this is a line highlight. I have actually seen clients that just make the text gray and it lo

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

sammccall added a comment. In D67536#1697696 , @nridge wrote: > One thing that may be worth considering as well, is that if the client > prefers to highlight the text of the line only, it can calculate the length > of the line itself. In VSCode for insta

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge added a comment. One thing that may be worth considering as well, is that if the client prefers to highlight the text of the line only, it can calculate the length of the line itself. In VSCode for instance, the line lengths are readily available; I imagine other editors are similar sinc

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

hokein added a comment. In D67536#1697533 , @nridge wrote: > How would one even measure the line length? `SourceManager` doesn't sem to > have a method like `getLineLength()` or similar. Yes, there is no existing API for that, I think you'd need to get

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

sammccall added a comment. In D67536#1697533 , @nridge wrote: > How would one even measure the line length? `SourceManager` doesn't sem to > have a method like `getLineLength()` or similar. If you look at functions like `offsetToPosition` in SourceCode.

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge marked 2 inline comments as done. nridge added a comment. How would one even measure the line length? `SourceManager` doesn't sem to have a method like `getLineLength()` or similar. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67536/new/ h

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge marked 5 inline comments as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/Compiler.cpp:66 CI->getLangOpts()->CommentOpts.ParseAllComments = true; + CI->getPreprocessorOpts().DetailedRecord = true; return CI; ilya-biryukov

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

sammccall added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:140 } +// Add inactive highlighting tokens. +const SourceManager &SM = AST.getSourceManager(); I think this comment could be clearer, e.g. // Add tokens in

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge updated this revision to Diff 223577. nridge added a comment. Update to use `PPCallbacks::SourceRangeSkipped` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67536/new/ https://reviews.llvm.org/D67536 Files: clang-tools-extra/clangd/ParsedA

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge added a comment. In D67536#1694098 , @MaskRay wrote: > Why "inactive region", not "skipped ranges"? I got the name from "$cquery/publishInactiveRegions" :) But I don't particularly care what we call it. Repository: rG LLVM Github Monorepo CH

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Compiler.cpp:66 CI->getLangOpts()->CommentOpts.ParseAllComments = true; + CI->getPreprocessorOpts().DetailedRecord = true; return CI; hokein wrote: > I'm not sure how does this flag

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

MaskRay added a comment. Why "inactive region", not "skipped ranges"? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67536/new/ https://reviews.llvm.org/D67536 ___ cfe-commits mailing list cfe-commits@l

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge marked an inline comment as done. nridge added a comment. In D67536#1691107 , @hokein wrote: > Could you add tests for this? Certainly, I just wanted to discuss the general approach first, as it will affect what the tests look like. ==

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

hokein added a comment. Could you add tests for this? Comment at: clang-tools-extra/clangd/Compiler.cpp:66 CI->getLangOpts()->CommentOpts.ParseAllComments = true; + CI->getPreprocessorOpts().DetailedRecord = true; return CI; I'm not sure how does this fl

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge added a comment. The updated patch formulates this as a new semantic highlighting kind. The tokens created for inactive regions are one per line, with the character offset and length being zero; the idea is that the client will handle this highlighting specially (see D67537

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge updated this revision to Diff 222336. nridge added a comment. Herald added subscribers: usaxena95, mgrang. Reformulate as an extension to semantic highlighting Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67536/new/ https://reviews.llvm.org

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge planned changes to this revision. nridge added a comment. Ok, I can try formulating this as part of / an extension to semantic highlighting. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67536/new/ https://reviews.llvm.org/D67536 ___

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

sammccall added a comment. Rather than a separate method with parallel implementation, this seems very closely related to the syntax highlighting feature. The minimal way to model this (no new protocol) would be for each disabled line, to add one token spanning the whole line with a TextMate sc

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge added a comment. For reference, I also posted a proof-of-concept client impl in D67537 , which I used to test/validate this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67536/new/ https://reviews.llv

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge created this revision. nridge added reviewers: sammccall, ilya-biryukov, hokein. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. nridge added a comment. This patch adds server-side support for greying out code in inactive preproce

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

nridge added a comment. This patch adds server-side support for greying out code in inactive preprocessor branches (issue #132 ). I didn't write test yet. I wanted to post the patch for feedback first, to see if the general approach is ok. Reposit