[PATCH] D67536: [clangd] Inactive regions support as an extension to semantic highlighting
This revision was automatically updated to reflect the committed changes. Closed by commit rGb2e6c2b9954b: [clangd] Inactive regions support as an extension to semantic highlighting (authored by nridge). Changed prior to commit: https://reviews.llvm.org/D67536?vs=230195&id=230565#toc 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/CollectMacros.h clang-tools-extra/clangd/Protocol.cpp clang-tools-extra/clangd/Protocol.h clang-tools-extra/clangd/SemanticHighlighting.cpp clang-tools-extra/clangd/SemanticHighlighting.h clang-tools-extra/clangd/test/semantic-highlighting.test clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp === --- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp +++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp @@ -140,7 +140,7 @@ } for (auto &LineTokens : ExpectedLines) ExpectedLinePairHighlighting.push_back( -{LineTokens.first, LineTokens.second}); +{LineTokens.first, LineTokens.second, /*IsInactive = */ false}); std::vector ActualDiffed = diffHighlightings(NewTokens, OldTokens); @@ -493,11 +493,11 @@ #define $Macro[[test]] #undef $Macro[[test]] - #ifdef $Macro[[test]] - #endif +$InactiveCode[[]] #ifdef $Macro[[test]] +$InactiveCode[[]] #endif - #if defined($Macro[[test]]) - #endif +$InactiveCode[[]] #if defined($Macro[[test]]) +$InactiveCode[[]] #endif )cpp", R"cpp( struct $Class[[S]] { @@ -598,6 +598,33 @@ $Class[[Foo]]<$TemplateParameter[[TT]], $TemplateParameter[[TTs]]...> *$Field[[t]]; } +)cpp", + // Inactive code highlighting + R"cpp( + // Code in the preamble. + // Inactive lines get an empty InactiveCode token at the beginning. +$InactiveCode[[]] #ifdef $Macro[[test]] +$InactiveCode[[]] #endif + + // A declaration to cause the preamble to end. + int $Variable[[EndPreamble]]; + + // Code after the preamble. + // Code inside inactive blocks does not get regular highlightings + // because it's not part of the AST. +$InactiveCode[[]] #ifdef $Macro[[test]] +$InactiveCode[[]] int Inactive2; +$InactiveCode[[]] #endif + + #ifndef $Macro[[test]] + int $Variable[[Active1]]; + #endif + +$InactiveCode[[]] #ifdef $Macro[[test]] +$InactiveCode[[]] int Inactive3; +$InactiveCode[[]] #else + int $Variable[[Active2]]; + #endif )cpp"}; for (const auto &TestCase : TestCases) { checkHighlightings(TestCase); @@ -665,10 +692,12 @@ {{HighlightingKind::Variable, Range{CreatePosition(3, 8), CreatePosition(3, 12)}}, {HighlightingKind::Function, - Range{CreatePosition(3, 4), CreatePosition(3, 7), + Range{CreatePosition(3, 4), CreatePosition(3, 7)}}}, + /* IsInactive = */ false}, {1, {{HighlightingKind::Variable, - Range{CreatePosition(1, 1), CreatePosition(1, 5)}; + Range{CreatePosition(1, 1), CreatePosition(1, 5)}}}, + /* IsInactive = */ true}}; std::vector ActualResults = toSemanticHighlightingInformation(Tokens); std::vector ExpectedResults = { Index: clang-tools-extra/clangd/test/semantic-highlighting.test === --- clang-tools-extra/clangd/test/semantic-highlighting.test +++ clang-tools-extra/clangd/test/semantic-highlighting.test @@ -57,6 +57,9 @@ # CHECK-NEXT: ], # CHECK-NEXT: [ # CHECK-NEXT:"entity.name.function.preprocessor.cpp" +# CHECK-NEXT: ], +# CHECK-NEXT: [ +# CHECK-NEXT:"meta.disabled" # CHECK-NEXT: ] # CHECK-NEXT:] # CHECK-NEXT: }, @@ -66,6 +69,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 0, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } @@ -81,10 +85,12 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 0, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 1, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } @@ -100,6 +106,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 1, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } @@ -115,6 +122,7 @@ # CHECK-NEXT: "params"
[PATCH] D67536: [clangd] Inactive regions support as an extension to semantic highlighting
hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. looks good, thanks! 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://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67536: [clangd] Inactive regions support as an extension to semantic highlighting
nridge updated this revision to Diff 230195. nridge marked 4 inline comments as done. nridge added a comment. Address 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/CollectMacros.h clang-tools-extra/clangd/Protocol.cpp clang-tools-extra/clangd/Protocol.h clang-tools-extra/clangd/SemanticHighlighting.cpp clang-tools-extra/clangd/SemanticHighlighting.h clang-tools-extra/clangd/test/semantic-highlighting.test clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp === --- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp +++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp @@ -140,7 +140,7 @@ } for (auto &LineTokens : ExpectedLines) ExpectedLinePairHighlighting.push_back( -{LineTokens.first, LineTokens.second}); +{LineTokens.first, LineTokens.second, /*IsInactive = */ false}); std::vector ActualDiffed = diffHighlightings(NewTokens, OldTokens); @@ -493,11 +493,11 @@ #define $Macro[[test]] #undef $Macro[[test]] - #ifdef $Macro[[test]] - #endif +$InactiveCode[[]] #ifdef $Macro[[test]] +$InactiveCode[[]] #endif - #if defined($Macro[[test]]) - #endif +$InactiveCode[[]] #if defined($Macro[[test]]) +$InactiveCode[[]] #endif )cpp", R"cpp( struct $Class[[S]] { @@ -598,6 +598,33 @@ $Class[[Foo]]<$TemplateParameter[[TT]], $TemplateParameter[[TTs]]...> *$Field[[t]]; } +)cpp", + // Inactive code highlighting + R"cpp( + // Code in the preamble. + // Inactive lines get an empty InactiveCode token at the beginning. +$InactiveCode[[]] #ifdef $Macro[[test]] +$InactiveCode[[]] #endif + + // A declaration to cause the preamble to end. + int $Variable[[EndPreamble]]; + + // Code after the preamble. + // Code inside inactive blocks does not get regular highlightings + // because it's not part of the AST. +$InactiveCode[[]] #ifdef $Macro[[test]] +$InactiveCode[[]] int Inactive2; +$InactiveCode[[]] #endif + + #ifndef $Macro[[test]] + int $Variable[[Active1]]; + #endif + +$InactiveCode[[]] #ifdef $Macro[[test]] +$InactiveCode[[]] int Inactive3; +$InactiveCode[[]] #else + int $Variable[[Active2]]; + #endif )cpp"}; for (const auto &TestCase : TestCases) { checkHighlightings(TestCase); @@ -665,10 +692,12 @@ {{HighlightingKind::Variable, Range{CreatePosition(3, 8), CreatePosition(3, 12)}}, {HighlightingKind::Function, - Range{CreatePosition(3, 4), CreatePosition(3, 7), + Range{CreatePosition(3, 4), CreatePosition(3, 7)}}}, + /* IsInactive = */ false}, {1, {{HighlightingKind::Variable, - Range{CreatePosition(1, 1), CreatePosition(1, 5)}; + Range{CreatePosition(1, 1), CreatePosition(1, 5)}}}, + /* IsInactive = */ true}}; std::vector ActualResults = toSemanticHighlightingInformation(Tokens); std::vector ExpectedResults = { Index: clang-tools-extra/clangd/test/semantic-highlighting.test === --- clang-tools-extra/clangd/test/semantic-highlighting.test +++ clang-tools-extra/clangd/test/semantic-highlighting.test @@ -57,6 +57,9 @@ # CHECK-NEXT: ], # CHECK-NEXT: [ # CHECK-NEXT:"entity.name.function.preprocessor.cpp" +# CHECK-NEXT: ], +# CHECK-NEXT: [ +# CHECK-NEXT:"meta.disabled" # CHECK-NEXT: ] # CHECK-NEXT:] # CHECK-NEXT: }, @@ -66,6 +69,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 0, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } @@ -81,10 +85,12 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 0, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 1, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } @@ -100,6 +106,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 1, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } @@ -115,6 +122,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 1, # CHECK-NEXT:"tokens": "
[PATCH] D67536: [clangd] Inactive regions support as an extension to semantic highlighting
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: > nridge wrote: > > hokein wrote: > > > it took me a while to understand this code, > > > > > > If the NewLine is `IsInactive`, it just contains a single token whose > > > range is [0, 0), can't we just? > > > > > > ``` > > > > > > if (NewLine.back().Tokens.empty()) continue; > > > > > > bool InactiveLine = NewLine.back().Tokens.front().Kind == InactiveCode; > > > assert(InactiveLine && NewLine.back().Tokens.size() == 1 && "IncativeCode > > > must have a single token"); > > > DiffedLines.back().IsInactive = true; > > > ``` > > An inactive line can contain token highlightings as well. For example, we > > highlight macro references in the condition of an `#ifdef`, and that line > > is also inactive if the condition is false. Clients can merge the line > > style (which is typically a background color) with the token styles > > (typically a foreground color). > > > > I did expand the comment to explain what the loop is doing more clearly. > thanks, I see. > > I think we can still simplify the code like below, this could improve the > code readability, and avoid the comment explaining it. > > ``` > llvm::erase_if(AddedLine.Tokens, [](const Token& T) { return T.Kind == > InactiveCode;}); > ``` Done. Note that we still need to set `AddedLine.IsInactive` appropriately. I did that in the lambda, which feels like a strange thing for an `erase_if` predicate to do. The alternative would be doing a `count_if` first (but then we're looping over the tokens twice). 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://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67536: [clangd] Inactive regions support as an extension to semantic highlighting
hokein added a comment. thanks, mostly good, a few more nits. Comment at: clang-tools-extra/clangd/ParsedAST.h:134 + // Ranges skipped during preprocessing. + std::vector SkippedRanges; }; nit: it is not used anymore, remove it. Comment at: clang-tools-extra/clangd/Protocol.h:1212 std::string Tokens; + /// Is the line in an inactive preprocessor branch? + bool IsInactive = false; hokein wrote: > could you add some documentation describing this is a clangd extension? could you document that the line-style highlightings can be combined with any token-style highlightings? Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:469 + auto &AddedLine = DiffedLines.back(); + for (auto Iter = AddedLine.Tokens.begin(); + Iter != AddedLine.Tokens.end();) { nridge wrote: > hokein wrote: > > it took me a while to understand this code, > > > > If the NewLine is `IsInactive`, it just contains a single token whose range > > is [0, 0), can't we just? > > > > ``` > > > > if (NewLine.back().Tokens.empty()) continue; > > > > bool InactiveLine = NewLine.back().Tokens.front().Kind == InactiveCode; > > assert(InactiveLine && NewLine.back().Tokens.size() == 1 && "IncativeCode > > must have a single token"); > > DiffedLines.back().IsInactive = true; > > ``` > An inactive line can contain token highlightings as well. For example, we > highlight macro references in the condition of an `#ifdef`, and that line is > also inactive if the condition is false. Clients can merge the line style > (which is typically a background color) with the token styles (typically a > foreground color). > > I did expand the comment to explain what the loop is doing more clearly. thanks, I see. I think we can still simplify the code like below, this could improve the code readability, and avoid the comment explaining it. ``` llvm::erase_if(AddedLine.Tokens, [](const Token& T) { return T.Kind == InactiveCode;}); ``` 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://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67536: [clangd] Inactive regions support as an extension to semantic highlighting
nridge updated this revision to Diff 229707. nridge added a comment. Support preamble as well 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/CollectMacros.h clang-tools-extra/clangd/ParsedAST.h clang-tools-extra/clangd/Protocol.cpp clang-tools-extra/clangd/Protocol.h clang-tools-extra/clangd/SemanticHighlighting.cpp clang-tools-extra/clangd/SemanticHighlighting.h clang-tools-extra/clangd/test/semantic-highlighting.test clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp === --- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp +++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp @@ -140,7 +140,7 @@ } for (auto &LineTokens : ExpectedLines) ExpectedLinePairHighlighting.push_back( -{LineTokens.first, LineTokens.second}); +{LineTokens.first, LineTokens.second, /*IsInactive = */ false}); std::vector ActualDiffed = diffHighlightings(NewTokens, OldTokens); @@ -493,11 +493,11 @@ #define $Macro[[test]] #undef $Macro[[test]] - #ifdef $Macro[[test]] - #endif +$InactiveCode[[]] #ifdef $Macro[[test]] +$InactiveCode[[]] #endif - #if defined($Macro[[test]]) - #endif +$InactiveCode[[]] #if defined($Macro[[test]]) +$InactiveCode[[]] #endif )cpp", R"cpp( struct $Class[[S]] { @@ -589,6 +589,33 @@ R"cpp( void $Function[[foo]](); using ::$Function[[foo]]; +)cpp", + // Inactive code highlighting + R"cpp( + // Code in the preamble. + // Inactive lines get an empty InactiveCode token at the beginning. +$InactiveCode[[]] #ifdef $Macro[[test]] +$InactiveCode[[]] #endif + + // A declaration to cause the preamble to end. + int $Variable[[EndPreamble]]; + + // Code after the preamble. + // Code inside inactive blocks does not get regular highlightings + // because it's not part of the AST. +$InactiveCode[[]] #ifdef $Macro[[test]] +$InactiveCode[[]] int Inactive2; +$InactiveCode[[]] #endif + + #ifndef $Macro[[test]] + int $Variable[[Active1]]; + #endif + +$InactiveCode[[]] #ifdef $Macro[[test]] +$InactiveCode[[]] int Inactive3; +$InactiveCode[[]] #else + int $Variable[[Active2]]; + #endif )cpp"}; for (const auto &TestCase : TestCases) { checkHighlightings(TestCase); @@ -656,10 +683,12 @@ {{HighlightingKind::Variable, Range{CreatePosition(3, 8), CreatePosition(3, 12)}}, {HighlightingKind::Function, - Range{CreatePosition(3, 4), CreatePosition(3, 7), + Range{CreatePosition(3, 4), CreatePosition(3, 7)}}}, + /* IsInactive = */ false}, {1, {{HighlightingKind::Variable, - Range{CreatePosition(1, 1), CreatePosition(1, 5)}; + Range{CreatePosition(1, 1), CreatePosition(1, 5)}}}, + /* IsInactive = */ true}}; std::vector ActualResults = toSemanticHighlightingInformation(Tokens); std::vector ExpectedResults = { Index: clang-tools-extra/clangd/test/semantic-highlighting.test === --- clang-tools-extra/clangd/test/semantic-highlighting.test +++ clang-tools-extra/clangd/test/semantic-highlighting.test @@ -57,6 +57,9 @@ # CHECK-NEXT: ], # CHECK-NEXT: [ # CHECK-NEXT:"entity.name.function.preprocessor.cpp" +# CHECK-NEXT: ], +# CHECK-NEXT: [ +# CHECK-NEXT:"meta.disabled" # CHECK-NEXT: ] # CHECK-NEXT:] # CHECK-NEXT: }, @@ -66,6 +69,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 0, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } @@ -81,10 +85,12 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 0, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 1, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } @@ -100,6 +106,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 1, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } @@ -115,6 +122,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 1, # CHECK-NEXT:"tokens": "" # CHECK-NEXT: }
[PATCH] D67536: [clangd] Inactive regions support as an extension to semantic highlighting
nridge marked an inline comment as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/ParsedAST.h:100 + const std::vector &getSkippedRanges() const { +return SkippedRanges; hokein wrote: > hokein wrote: > > Instead of adding new member and methods in `ParsedAST`, I think we can do > > it in `CollectMainFileMacros` (adding a new field SkippRanges in > > `MainFileMacros`), then we can get the skipped ranges for preamble region > > within the main file for free. > This comment is undone. My apologies, I overlooked this. 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://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67536: [clangd] Inactive regions support as an extension to semantic highlighting
hokein added inline comments. Comment at: clang-tools-extra/clangd/ParsedAST.h:100 + const std::vector &getSkippedRanges() const { +return SkippedRanges; hokein wrote: > Instead of adding new member and methods in `ParsedAST`, I think we can do it > in `CollectMainFileMacros` (adding a new field SkippRanges in > `MainFileMacros`), then we can get the skipped ranges for preamble region > within the main file for free. This comment is undone. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:143 +for (const SourceRange &R : + AST.getPreprocessor().getPreprocessingRecord()->getSkippedRanges()) { + if (isInsideMainFile(R.getBegin(), SM)) { nridge wrote: > hokein wrote: > > I think the current implementation only collects the skipped preprocessing > > ranges **after preamble** region in the main file. We probably need to > > store these ranges in PreambleData to make the ranges in the preamble > > region work, no need to do it in this patch, but we'd better have a test > > reflecting this fact. > > > > ``` > > #ifdef ActiveCOde > > // inactive code here > > #endif > > > > #include "foo.h" > > // preamble ends here > > > > namespace ns { > > // .. > > } > > ``` > Your observation is correct: the current implementation only highlights > inactive lines after the preamble. For now, I added a test case with a FIXME > for this. I'd prefer to fix it in this patch, it should not require large effort, and probably simplify the patch, you don't need to implement a new `PPCallbacks`. See my another comment. 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://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67536: [clangd] Inactive regions support as an extension to semantic highlighting
nridge updated this revision to Diff 228959. nridge added a comment. Add unit tests 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/ParsedAST.h clang-tools-extra/clangd/Protocol.cpp clang-tools-extra/clangd/Protocol.h clang-tools-extra/clangd/SemanticHighlighting.cpp clang-tools-extra/clangd/SemanticHighlighting.h clang-tools-extra/clangd/test/semantic-highlighting.test clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp === --- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp +++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp @@ -140,7 +140,7 @@ } for (auto &LineTokens : ExpectedLines) ExpectedLinePairHighlighting.push_back( -{LineTokens.first, LineTokens.second}); +{LineTokens.first, LineTokens.second, /*IsInactive = */ false}); std::vector ActualDiffed = diffHighlightings(NewTokens, OldTokens); @@ -589,6 +589,33 @@ R"cpp( void $Function[[foo]](); using ::$Function[[foo]]; +)cpp", + // Inactive code highlighting + R"cpp( + // FIXME: In the preamble, no inactive code highlightings are produced. + #ifdef $Macro[[test]] + #endif + + // A declaration to cause the preamble to end. + int $Variable[[EndPreamble]]; + + // Code after the preamble. + // Inactive lines get an empty InactiveCode token at the beginning. + // Code inside inactive blocks does not get regular highlightings + // because it's not part of the AST. +$InactiveCode[[]] #ifdef $Macro[[test]] +$InactiveCode[[]] int Inactive1; +$InactiveCode[[]] #endif + + #ifndef $Macro[[test]] + int $Variable[[Active1]]; + #endif + +$InactiveCode[[]] #ifdef $Macro[[test]] +$InactiveCode[[]] int Inactive2; +$InactiveCode[[]] #else + int $Variable[[Active2]]; + #endif )cpp"}; for (const auto &TestCase : TestCases) { checkHighlightings(TestCase); @@ -656,10 +683,12 @@ {{HighlightingKind::Variable, Range{CreatePosition(3, 8), CreatePosition(3, 12)}}, {HighlightingKind::Function, - Range{CreatePosition(3, 4), CreatePosition(3, 7), + Range{CreatePosition(3, 4), CreatePosition(3, 7)}}}, + /* IsInactive = */ false}, {1, {{HighlightingKind::Variable, - Range{CreatePosition(1, 1), CreatePosition(1, 5)}; + Range{CreatePosition(1, 1), CreatePosition(1, 5)}}}, + /* IsInactive = */ true}}; std::vector ActualResults = toSemanticHighlightingInformation(Tokens); std::vector ExpectedResults = { Index: clang-tools-extra/clangd/test/semantic-highlighting.test === --- clang-tools-extra/clangd/test/semantic-highlighting.test +++ clang-tools-extra/clangd/test/semantic-highlighting.test @@ -57,6 +57,9 @@ # CHECK-NEXT: ], # CHECK-NEXT: [ # CHECK-NEXT:"entity.name.function.preprocessor.cpp" +# CHECK-NEXT: ], +# CHECK-NEXT: [ +# CHECK-NEXT:"meta.disabled" # CHECK-NEXT: ] # CHECK-NEXT:] # CHECK-NEXT: }, @@ -66,6 +69,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 0, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } @@ -81,10 +85,12 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 0, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 1, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } @@ -100,6 +106,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 1, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } @@ -115,6 +122,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 1, # CHECK-NEXT:"tokens": "" # CHECK-NEXT: } Index: clang-tools-extra/clangd/SemanticHighlighting.h === --- clang-tools-extra/clangd/SemanticHighlighting.h +++ clang-tools-extra/clangd/SemanticHighlighting.h @@ -44,7 +44,11 @@ Primitive, Macro, - LastKind = Macro + // This one is different from the other kinds as it's a line style + // rather than a token st
[PATCH] D67536: [clangd] Inactive regions support as an extension to semantic highlighting
nridge added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:143 +for (const SourceRange &R : + AST.getPreprocessor().getPreprocessingRecord()->getSkippedRanges()) { + if (isInsideMainFile(R.getBegin(), SM)) { hokein wrote: > I think the current implementation only collects the skipped preprocessing > ranges **after preamble** region in the main file. We probably need to store > these ranges in PreambleData to make the ranges in the preamble region work, > no need to do it in this patch, but we'd better have a test reflecting this > fact. > > ``` > #ifdef ActiveCOde > // inactive code here > #endif > > #include "foo.h" > // preamble ends here > > namespace ns { > // .. > } > ``` Your observation is correct: the current implementation only highlights inactive lines after the preamble. For now, I added a test case with a FIXME for this. 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://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67536: [clangd] Inactive regions support as an extension to semantic highlighting
nridge updated this revision to Diff 228960. nridge marked 11 inline comments as done. nridge added a comment. Address one minor remaining comment 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/ParsedAST.h clang-tools-extra/clangd/Protocol.cpp clang-tools-extra/clangd/Protocol.h clang-tools-extra/clangd/SemanticHighlighting.cpp clang-tools-extra/clangd/SemanticHighlighting.h clang-tools-extra/clangd/test/semantic-highlighting.test clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp === --- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp +++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp @@ -140,7 +140,7 @@ } for (auto &LineTokens : ExpectedLines) ExpectedLinePairHighlighting.push_back( -{LineTokens.first, LineTokens.second}); +{LineTokens.first, LineTokens.second, /*IsInactive = */ false}); std::vector ActualDiffed = diffHighlightings(NewTokens, OldTokens); @@ -589,6 +589,33 @@ R"cpp( void $Function[[foo]](); using ::$Function[[foo]]; +)cpp", + // Inactive code highlighting + R"cpp( + // FIXME: In the preamble, no inactive code highlightings are produced. + #ifdef $Macro[[test]] + #endif + + // A declaration to cause the preamble to end. + int $Variable[[EndPreamble]]; + + // Code after the preamble. + // Inactive lines get an empty InactiveCode token at the beginning. + // Code inside inactive blocks does not get regular highlightings + // because it's not part of the AST. +$InactiveCode[[]] #ifdef $Macro[[test]] +$InactiveCode[[]] int Inactive1; +$InactiveCode[[]] #endif + + #ifndef $Macro[[test]] + int $Variable[[Active1]]; + #endif + +$InactiveCode[[]] #ifdef $Macro[[test]] +$InactiveCode[[]] int Inactive2; +$InactiveCode[[]] #else + int $Variable[[Active2]]; + #endif )cpp"}; for (const auto &TestCase : TestCases) { checkHighlightings(TestCase); @@ -656,10 +683,12 @@ {{HighlightingKind::Variable, Range{CreatePosition(3, 8), CreatePosition(3, 12)}}, {HighlightingKind::Function, - Range{CreatePosition(3, 4), CreatePosition(3, 7), + Range{CreatePosition(3, 4), CreatePosition(3, 7)}}}, + /* IsInactive = */ false}, {1, {{HighlightingKind::Variable, - Range{CreatePosition(1, 1), CreatePosition(1, 5)}; + Range{CreatePosition(1, 1), CreatePosition(1, 5)}}}, + /* IsInactive = */ true}}; std::vector ActualResults = toSemanticHighlightingInformation(Tokens); std::vector ExpectedResults = { Index: clang-tools-extra/clangd/test/semantic-highlighting.test === --- clang-tools-extra/clangd/test/semantic-highlighting.test +++ clang-tools-extra/clangd/test/semantic-highlighting.test @@ -57,6 +57,9 @@ # CHECK-NEXT: ], # CHECK-NEXT: [ # CHECK-NEXT:"entity.name.function.preprocessor.cpp" +# CHECK-NEXT: ], +# CHECK-NEXT: [ +# CHECK-NEXT:"meta.disabled" # CHECK-NEXT: ] # CHECK-NEXT:] # CHECK-NEXT: }, @@ -66,6 +69,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 0, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } @@ -81,10 +85,12 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 0, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 1, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } @@ -100,6 +106,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 1, # CHECK-NEXT:"tokens": "BAABAAA=" # CHECK-NEXT: } @@ -115,6 +122,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT:"lines": [ # CHECK-NEXT: { +# CHECK-NEXT:"isInactive": false, # CHECK-NEXT:"line": 1, # CHECK-NEXT:"tokens": "" # CHECK-NEXT: } Index: clang-tools-extra/clangd/SemanticHighlighting.h === --- clang-tools-extra/clangd/SemanticHighlighting.h +++ clang-tools-extra/clangd/SemanticHighlighting.h @@ -44,7 +44,11 @@ Primitive, Macro, - LastKind = Macro + // This one is different from th