[clang-tools-extra] 8ec4498 - [clangd] Do not end inactiveRegions range at position 0 of line
Nathan Ridge via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 5 00:51:28 PDT 2023
Author: Nathan Ridge
Date: 2023-06-05T03:51:15-04:00
New Revision: 8ec44987e54b5366a03716237feb79e37ae0634a
URL: https://github.com/llvm/llvm-project/commit/8ec44987e54b5366a03716237feb79e37ae0634a
DIFF: https://github.com/llvm/llvm-project/commit/8ec44987e54b5366a03716237feb79e37ae0634a.diff
LOG: [clangd] Do not end inactiveRegions range at position 0 of line
This carries over the fix previously made for semantic highlighting
https://reviews.llvm.org/D92148, to the new inactiveRegions
protocol as well.
In addition, the directives at the beginning and end of an
inactive region are now excluded from the region.
Fixes https://github.com/clangd/clangd/issues/1631
Fixes https://github.com/clangd/clangd/issues/773
Differential Revision: https://reviews.llvm.org/D151190
Added:
Modified:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/SemanticHighlighting.cpp
clang-tools-extra/clangd/SemanticHighlighting.h
clang-tools-extra/clangd/unittests/ClangdTests.cpp
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 7c5042b8414b4..cd3a52249dfb7 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -116,8 +116,8 @@ struct UpdateIndexCallbacks : public ParsingCallbacks {
ServerCallbacks->onDiagnosticsReady(Path, AST.version(),
std::move(Diagnostics));
if (CollectInactiveRegions) {
- ServerCallbacks->onInactiveRegionsReady(
- Path, std::move(AST.getMacros().SkippedRanges));
+ ServerCallbacks->onInactiveRegionsReady(Path,
+ getInactiveRegions(AST));
}
});
}
diff --git a/clang-tools-extra/clangd/SemanticHighlighting.cpp b/clang-tools-extra/clangd/SemanticHighlighting.cpp
index 34a0214b082bd..ec37476cf94ea 100644
--- a/clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ b/clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -39,6 +39,17 @@ namespace clang {
namespace clangd {
namespace {
+/// Get the last Position on a given line.
+llvm::Expected<Position> endOfLine(llvm::StringRef Code, int Line) {
+ auto StartOfLine = positionToOffset(Code, Position{Line, 0});
+ if (!StartOfLine)
+ return StartOfLine.takeError();
+ StringRef LineText = Code.drop_front(*StartOfLine).take_until([](char C) {
+ return C == '\n';
+ });
+ return Position{Line, static_cast<int>(lspLength(LineText))};
+}
+
/// Some names are not written in the source code and cannot be highlighted,
/// e.g. anonymous classes. This function detects those cases.
bool canHighlightName(DeclarationName Name) {
@@ -516,38 +527,27 @@ class HighlightingsBuilder {
// Merge token stream with "inactive line" markers.
std::vector<HighlightingToken> WithInactiveLines;
- auto SortedSkippedRanges = AST.getMacros().SkippedRanges;
- llvm::sort(SortedSkippedRanges);
+ auto SortedInactiveRegions = getInactiveRegions(AST);
+ llvm::sort(SortedInactiveRegions);
auto It = NonConflicting.begin();
- for (const Range &R : SortedSkippedRanges) {
- // Create one token for each line in the skipped range, so it works
+ for (const Range &R : SortedInactiveRegions) {
+ // Create one token for each line in the inactive range, so it works
// with line-based
diff ing.
assert(R.start.line <= R.end.line);
for (int Line = R.start.line; Line <= R.end.line; ++Line) {
- // If the end of the inactive range is at the beginning
- // of a line, that line is not inactive.
- if (Line == R.end.line && R.end.character == 0)
- continue;
// Copy tokens before the inactive line
for (; It != NonConflicting.end() && It->R.start.line < Line; ++It)
WithInactiveLines.push_back(std::move(*It));
// Add a token for the inactive line itself.
- auto StartOfLine = positionToOffset(MainCode, Position{Line, 0});
- if (StartOfLine) {
- StringRef LineText =
- MainCode.drop_front(*StartOfLine).take_until([](char C) {
- return C == '\n';
- });
+ auto EndOfLine = endOfLine(MainCode, Line);
+ if (EndOfLine) {
HighlightingToken HT;
WithInactiveLines.emplace_back();
WithInactiveLines.back().Kind = HighlightingKind::InactiveCode;
WithInactiveLines.back().R.start.line = Line;
- WithInactiveLines.back().R.end.line = Line;
- WithInactiveLines.back().R.end.character =
- static_cast<int>(lspLength(LineText));
+ WithInactiveLines.back().R.end = *EndOfLine;
} else {
- elog("Failed to convert position to offset: {0}",
- StartOfLine.takeError());
+ elog("Failed to determine end of line: {0}", EndOfLine.takeError());
}
// Skip any other tokens on the inactive line. e.g.
@@ -1544,5 +1544,40 @@
diff Tokens(llvm::ArrayRef<SemanticToken> Old,
return {std::move(Edit)};
}
+std::vector<Range> getInactiveRegions(ParsedAST &AST) {
+ std::vector<Range> SkippedRanges(std::move(AST.getMacros().SkippedRanges));
+ const auto &SM = AST.getSourceManager();
+ StringRef MainCode = SM.getBufferOrFake(SM.getMainFileID()).getBuffer();
+ std::vector<Range> InactiveRegions;
+ for (const Range &Skipped : SkippedRanges) {
+ Range Inactive = Skipped;
+ // Sometimes, SkippedRanges contains a range ending at position 0
+ // of a line. Clients that apply whole-line styles will treat that
+ // line as inactive which is not desirable, so adjust the ending
+ // position to be the end of the previous line.
+ if (Inactive.end.character == 0 && Inactive.end.line > 0) {
+ --Inactive.end.line;
+ }
+ // Exclude the directive lines themselves from the range.
+ if (Inactive.end.line >= Inactive.start.line + 2) {
+ ++Inactive.start.line;
+ --Inactive.end.line;
+ } else {
+ // range would be empty, e.g. #endif on next line after #ifdef
+ continue;
+ }
+ // Since we've adjusted the ending line, we need to recompute the
+ // column to reflect the end of that line.
+ if (auto EndOfLine = endOfLine(MainCode, Inactive.end.line)) {
+ Inactive.end = *EndOfLine;
+ } else {
+ elog("Failed to determine end of line: {0}", EndOfLine.takeError());
+ continue;
+ }
+ InactiveRegions.push_back(Inactive);
+ }
+ return InactiveRegions;
+}
+
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/SemanticHighlighting.h b/clang-tools-extra/clangd/SemanticHighlighting.h
index ca90230dfb8fb..c9db598ff08c9 100644
--- a/clang-tools-extra/clangd/SemanticHighlighting.h
+++ b/clang-tools-extra/clangd/SemanticHighlighting.h
@@ -120,6 +120,11 @@ llvm::StringRef toSemanticTokenModifier(HighlightingModifier Modifier);
std::vector<SemanticTokensEdit>
diff Tokens(llvm::ArrayRef<SemanticToken> Before,
llvm::ArrayRef<SemanticToken> After);
+// Returns ranges of the file that are inside an inactive preprocessor branch.
+// The preprocessor directives at the beginning and end of a branch themselves
+// are not included.
+std::vector<Range> getInactiveRegions(ParsedAST &AST);
+
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/unittests/ClangdTests.cpp b/clang-tools-extra/clangd/unittests/ClangdTests.cpp
index c6b464fb78746..be6c2fba12d1c 100644
--- a/clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -1332,26 +1332,31 @@ TEST(ClangdServer, InactiveRegions) {
#define PREAMBLEMACRO 42
#if PREAMBLEMACRO > 40
#define ACTIVE
-$inactive1[[#else
- #define INACTIVE
-#endif]]
+#else
+$inactive1[[ #define INACTIVE]]
+#endif
int endPreamble;
-$inactive2[[#ifndef CMDMACRO
- int inactiveInt;
-#endif]]
+#ifndef CMDMACRO
+$inactive2[[ int inactiveInt;]]
+#endif
#undef CMDMACRO
-$inactive3[[#ifdef CMDMACRO
- int inactiveInt2;
-#else]]
- int activeInt;
+#ifdef CMDMACRO
+$inactive3[[ int inactiveInt2;]]
+#elif PREAMBLEMACRO > 0
+ int activeInt1;
+ int activeInt2;
+#else
+$inactive4[[ int inactiveInt3;]]
#endif
+#ifdef CMDMACRO
+#endif // empty inactive range, gets dropped
)cpp");
Server.addDocument(testPath("foo.cpp"), Source.code());
ASSERT_TRUE(Server.blockUntilIdleForTest());
EXPECT_THAT(Callback.FoundInactiveRegions,
- ElementsAre(ElementsAre(Source.range("inactive1"),
- Source.range("inactive2"),
- Source.range("inactive3"))));
+ ElementsAre(ElementsAre(
+ Source.range("inactive1"), Source.range("inactive2"),
+ Source.range("inactive3"), Source.range("inactive4"))));
}
} // namespace
diff --git a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
index c25dff810b764..9c6e5246f5c37 100644
--- a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -451,11 +451,11 @@ TEST(SemanticHighlighting, GetsCorrectTokens) {
#define $Macro_decl[[test]]
#undef $Macro[[test]]
-$InactiveCode[[#ifdef test]]
-$InactiveCode[[#endif]]
+ #ifdef $Macro[[test]]
+ #endif
-$InactiveCode[[#if defined(test)]]
-$InactiveCode[[#endif]]
+ #if defined($Macro[[test]])
+ #endif
)cpp",
R"cpp(
struct $Class_def[[S]] {
@@ -562,8 +562,9 @@ TEST(SemanticHighlighting, GetsCorrectTokens) {
R"cpp(
// Code in the preamble.
// Inactive lines get an empty InactiveCode token at the beginning.
-$InactiveCode[[#ifdef test]]
-$InactiveCode[[#endif]]
+ #ifdef $Macro[[test]]
+$InactiveCode[[int Inactive1;]]
+ #endif
// A declaration to cause the preamble to end.
int $Variable_def[[EndPreamble]];
@@ -572,21 +573,21 @@ TEST(SemanticHighlighting, GetsCorrectTokens) {
// Code inside inactive blocks does not get regular highlightings
// because it's not part of the AST.
#define $Macro_decl[[test2]]
-$InactiveCode[[#if defined(test)]]
+ #if defined($Macro[[test]])
$InactiveCode[[int Inactive2;]]
-$InactiveCode[[#elif defined(test2)]]
+ #elif defined($Macro[[test2]])
int $Variable_def[[Active1]];
-$InactiveCode[[#else]]
+ #else
$InactiveCode[[int Inactive3;]]
-$InactiveCode[[#endif]]
+ #endif
#ifndef $Macro[[test]]
int $Variable_def[[Active2]];
#endif
-$InactiveCode[[#ifdef test]]
+ #ifdef $Macro[[test]]
$InactiveCode[[int Inactive4;]]
-$InactiveCode[[#else]]
+ #else
int $Variable_def[[Active3]];
#endif
)cpp",
More information about the cfe-commits
mailing list