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", _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
