[PATCH] D151190: [clangd] Do not end inactiveRegions range at position 0 of line
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8ec44987e54b: [clangd] Do not end inactiveRegions range at
position 0 of line (authored by nridge).
Changed prior to commit:
https://reviews.llvm.org/D151190?vs=528190&id=528304#toc
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/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
@@ -451,11 +451,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]] {
@@ -562,8 +562,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]];
@@ -572,21 +573,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/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -120,6 +120,11 @@
std::vector diffTokens(llvm::ArrayRef Before,
llvm::ArrayRef 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 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
@@ -39,6 +39,17 @@
namespace clangd {
namespace {
+/// Get the last Position on a given line.
+llvm::Expected endOfLine(llvm::StringRef Code, int Line) {
+ auto StartOfLine =
[PATCH] D151190: [clangd] Do not end inactiveRegions range at position 0 of line
hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. thanks, looks great. Comment at: clang-tools-extra/clangd/SourceCode.h:76 +/// Get the last Position on a given line. +llvm::Expected endOfLine(llvm::StringRef Code, int Line); + nit: no need to expose this API, it is only used in SemanticHighlighting.cpp. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151190/new/ https://reviews.llvm.org/D151190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D151190: [clangd] Do not end inactiveRegions range at position 0 of line
nridge updated this revision to Diff 528190.
nridge added a comment.
Add comment
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 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 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 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([](
[PATCH] D151190: [clangd] Do not end inactiveRegions range at position 0 of line
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 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 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 endOfLine(llvm::StringRef Code, int Line) {
+ auto StartOfLine = positionToOffset(Code, Position{Line, 0});
+ if (!StartOfLine)
+return Start
[PATCH] D151190: [clangd] Do not end inactiveRegions range at position 0 of line
hokein added inline comments.
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:119
if (CollectInactiveRegions) {
- ServerCallbacks->onInactiveRegionsReady(
- Path, std::move(AST.getMacros().SkippedRanges));
+ std::vector SkippedRanges(
+ std::move(AST.getMacros().SkippedRanges));
this part of code becomes non-trivial now, I suggest pulling out a function and
moving it to `SemanticHighlighting.cpp`. The old inactive-as-comment
implementation can share it as well.
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:153
+ }
+ ServerCallbacks->onInactiveRegionsReady(Path, InactiveRegions);
}
nit: std::move(InactiveRegions).
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:478
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.
I think we should do the same thing for the old implementation as well (or just
delete it at all), otherwise, we will have inconsistent behavior.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151190/new/
https://reviews.llvm.org/D151190
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D151190: [clangd] Do not end inactiveRegions range at position 0 of line
nridge updated this revision to Diff 526409.
nridge added a comment.
slight simplification
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/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/unittests/ClangdTests.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 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 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 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(lspLength(LineText))};
+}
+
bool isSpelledInSource(SourceLocation Loc, const SourceManager &SM) {
if (Loc.isFileID())
return true;
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -483,22 +483,15 @@
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(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.
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clang
[PATCH] D151190: [clangd] Do not end inactiveRegions range at position 0 of line
nridge updated this revision to Diff 526408.
nridge added a comment.
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/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/unittests/ClangdTests.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 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 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 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(lspLength(LineText))};
+}
+
bool isSpelledInSource(SourceLocation Loc, const SourceManager &SM) {
if (Loc.isFileID())
return true;
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -483,22 +483,15 @@
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(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.
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/cla
[PATCH] D151190: [clangd] Do not end inactiveRegions range at position 0 of line
nridge planned changes to this revision. nridge added inline comments. Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:1343 #undef CMDMACRO $inactive3[[#ifdef CMDMACRO int inactiveInt2; daiyousei-qz wrote: > hokein wrote: > > While this patch is an improvement, I wonder we should move it further. > > > > Has been thinking about it more, we seem to have some inconsistent behavior > > on highlighting the `#if`, `#else`, `#endif` lines: > > > > - in `$inactive1` case, the `#endif` is marked as inactive (IMO, the > > highlighting in the editor is confusing, it looks like we're missing a > > match `endif`, thus I prefer to mark it as active to correspond to the > > active `#if` branch); > > - in `$inactive3` case, the line `#elif PREAMBLEMACRO > 0` is marked as > > inactive, this is inconsistent with `$inactive1` case; > > > > I think it would be nice to have a consistent model. One approach is to > > always consider `#if`, `#elif`, `#endif`, `#endif` lines active (in the > > implementation side, this would mean we always use the line range > > [skipp_range.start.line+1, skipp_range.end.line - 1]). > > > > What do you think about this? > > > > > +1. My perspective is that C++ source code is actually a meta-language > (preprocessor) that describes generation of C++ language code. That is, > `#if`, `#else`, `#endif` and .etc are always in a sense "executed" to > generate the actual code. So they shouldn't be marked as inactive. I agree, what you describe is a nice additional improvement. I believe it would also resolve the long-standing issue https://github.com/clangd/clangd/issues/773. I will update the patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151190/new/ https://reviews.llvm.org/D151190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D151190: [clangd] Do not end inactiveRegions range at position 0 of line
daiyousei-qz added inline comments. Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:1343 #undef CMDMACRO $inactive3[[#ifdef CMDMACRO int inactiveInt2; hokein wrote: > While this patch is an improvement, I wonder we should move it further. > > Has been thinking about it more, we seem to have some inconsistent behavior > on highlighting the `#if`, `#else`, `#endif` lines: > > - in `$inactive1` case, the `#endif` is marked as inactive (IMO, the > highlighting in the editor is confusing, it looks like we're missing a match > `endif`, thus I prefer to mark it as active to correspond to the active `#if` > branch); > - in `$inactive3` case, the line `#elif PREAMBLEMACRO > 0` is marked as > inactive, this is inconsistent with `$inactive1` case; > > I think it would be nice to have a consistent model. One approach is to > always consider `#if`, `#elif`, `#endif`, `#endif` lines active (in the > implementation side, this would mean we always use the line range > [skipp_range.start.line+1, skipp_range.end.line - 1]). > > What do you think about this? > > +1. My perspective is that C++ source code is actually a meta-language (preprocessor) that describes generation of C++ language code. That is, `#if`, `#else`, `#endif` and .etc are always in a sense "executed" to generate the actual code. So they shouldn't be marked as inactive. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151190/new/ https://reviews.llvm.org/D151190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D151190: [clangd] Do not end inactiveRegions range at position 0 of line
hokein added inline comments. Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:1343 #undef CMDMACRO $inactive3[[#ifdef CMDMACRO int inactiveInt2; While this patch is an improvement, I wonder we should move it further. Has been thinking about it more, we seem to have some inconsistent behavior on highlighting the `#if`, `#else`, `#endif` lines: - in `$inactive1` case, the `#endif` is marked as inactive (IMO, the highlighting in the editor is confusing, it looks like we're missing a match `endif`, thus I prefer to mark it as active to correspond to the active `#if` branch); - in `$inactive3` case, the line `#elif PREAMBLEMACRO > 0` is marked as inactive, this is inconsistent with `$inactive1` case; I think it would be nice to have a consistent model. One approach is to always consider `#if`, `#elif`, `#endif`, `#endif` lines active (in the implementation side, this would mean we always use the line range [skipp_range.start.line+1, skipp_range.end.line - 1]). What do you think about this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151190/new/ https://reviews.llvm.org/D151190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D151190: [clangd] Do not end inactiveRegions range at position 0 of line
nridge created this revision.
nridge added a reviewer: hokein.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
nridge requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
This carries over the fix previously made for semantic highlighting
https://reviews.llvm.org/D92148, to the new inactiveRegions
protocol as well.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D151190
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/SemanticHighlighting.cpp
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/unittests/ClangdTests.cpp
Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -1342,16 +1342,19 @@
#undef CMDMACRO
$inactive3[[#ifdef CMDMACRO
int inactiveInt2;
-#else]]
- int activeInt;
-#endif
+#elif PREAMBLEMACRO > 0]]
+ int activeInt1;
+ int activeInt2;
+$inactive4[[#else
+ int inactiveInt3;
+#endif]]
)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 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 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 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(lspLength(LineText))};
+}
+
bool isSpelledInSource(SourceLocation Loc, const SourceManager &SM) {
if (Loc.isFileID())
return true;
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -483,22 +483,15 @@
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(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.
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -116,8 +116,26 @@
ServerCallbacks->onDiagnosticsReady(Path, AST.version(),
