[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=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 : 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 : 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:
[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 : 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 : 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": ""
# CHECK-NEXT:
[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 = 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 = 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 : 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 : 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:
[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 () 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 () 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 :
+ 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 : 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 : 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 style.
+
[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 :
+ 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 : 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 : 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
