nridge updated this revision to Diff 528189.
nridge marked 3 inline comments as done.
nridge added a comment.
Herald added a subscriber: mgrang.
Address review comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151190/new/
https://reviews.llvm.org/D151190
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/SemanticHighlighting.cpp
clang-tools-extra/clangd/SemanticHighlighting.h
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/unittests/ClangdTests.cpp
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
@@ -450,11 +450,11 @@
#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]] {
@@ -561,8 +561,9 @@
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]];
@@ -571,21 +572,21 @@
// 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",
Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -1332,26 +1332,31 @@
#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
Index: clang-tools-extra/clangd/SourceCode.h
===================================================================
--- clang-tools-extra/clangd/SourceCode.h
+++ clang-tools-extra/clangd/SourceCode.h
@@ -72,6 +72,9 @@
/// FIXME: This should return an error if the location is invalid.
Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc);
+/// Get the last Position on a given line.
+llvm::Expected<Position> endOfLine(llvm::StringRef Code, int Line);
+
/// Return the file location, corresponding to \p P. Note that one should take
/// care to avoid comparing the result with expansion locations.
llvm::Expected<SourceLocation> sourceLocationInMainFile(const SourceManager &SM,
Index: clang-tools-extra/clangd/SourceCode.cpp
===================================================================
--- clang-tools-extra/clangd/SourceCode.cpp
+++ clang-tools-extra/clangd/SourceCode.cpp
@@ -228,6 +228,16 @@
return P;
}
+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))};
+}
+
bool isSpelledInSource(SourceLocation Loc, const SourceManager &SM) {
if (Loc.isFileID())
return true;
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===================================================================
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -116,6 +116,8 @@
std::vector<SemanticTokensEdit> diffTokens(llvm::ArrayRef<SemanticToken> Before,
llvm::ArrayRef<SemanticToken> After);
+std::vector<Range> getInactiveRegions(ParsedAST &AST);
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===================================================================
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -467,38 +467,27 @@
// 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 diffing.
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.
@@ -1433,5 +1422,40 @@
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
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -116,8 +116,8 @@
ServerCallbacks->onDiagnosticsReady(Path, AST.version(),
std::move(Diagnostics));
if (CollectInactiveRegions) {
- ServerCallbacks->onInactiveRegionsReady(
- Path, std::move(AST.getMacros().SkippedRanges));
+ ServerCallbacks->onInactiveRegionsReady(Path,
+ getInactiveRegions(AST));
}
});
}
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
