jvikstrom created this revision.
jvikstrom added reviewers: hokein, ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay.
Herald added a project: clang.
It is possible to write include code from other files so that the decls from
there do not become topLevelDecls (For example by including methods for a
class). These Decls are not filtered by topLevelDecls and therefore
SemanticHighlighting must manually check that every SLoc belongs in the main
file. Otherwise there can be highlightings appearing in places where they
should not.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D66083
Files:
clang-tools-extra/clangd/SemanticHighlighting.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
@@ -51,9 +51,16 @@
return ExpectedTokens;
}
-void checkHighlightings(llvm::StringRef Code) {
+// AdditionalFiles are key-value-pair that are included in the TU where the key
+// is the filename and the value is the code for the file.
+void checkHighlightings(llvm::StringRef Code,
+ std::vector<std::pair<llvm::StringRef,
llvm::StringRef>>
+ AdditionalFiles = {}) {
Annotations Test(Code);
- auto AST = TestTU::withCode(Test.code()).build();
+ auto TU = TestTU::withCode(Test.code());
+ for (auto File : AdditionalFiles)
+ TU.AdditionalFiles.insert({File.first, File.second});
+ auto AST = TU.build();
std::vector<HighlightingToken> ActualTokens = getSemanticHighlightings(AST);
EXPECT_THAT(ActualTokens, getExpectedTokens(Test)) << Code;
}
@@ -321,6 +328,17 @@
for (const auto &TestCase : TestCases) {
checkHighlightings(TestCase);
}
+
+ checkHighlightings(R"cpp(
+ class $Class[[A]] {
+ #include "imp.h"
+ };
+ #endif
+ )cpp",
+ {{"imp.h", R"cpp(
+ int someMethod();
+ void otherMethod();
+ )cpp"}});
}
TEST(SemanticHighlighting, GeneratesHighlightsWhenFileChange) {
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===================================================================
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -231,6 +231,12 @@
// FIXME: skip tokens inside macros for now.
return;
+ // Non top level decls that are included from a header are not filtered by
+ // topLevelDecls. (example: method declarations being included from another
+ // file for a class) from another file)
+ if (!SM.isWrittenInMainFile(Loc))
+ return;
+
auto R = getTokenRange(SM, Ctx.getLangOpts(), Loc);
if (!R) {
// R should always have a value, if it doesn't something is very wrong.
Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -51,9 +51,16 @@
return ExpectedTokens;
}
-void checkHighlightings(llvm::StringRef Code) {
+// AdditionalFiles are key-value-pair that are included in the TU where the key
+// is the filename and the value is the code for the file.
+void checkHighlightings(llvm::StringRef Code,
+ std::vector<std::pair<llvm::StringRef, llvm::StringRef>>
+ AdditionalFiles = {}) {
Annotations Test(Code);
- auto AST = TestTU::withCode(Test.code()).build();
+ auto TU = TestTU::withCode(Test.code());
+ for (auto File : AdditionalFiles)
+ TU.AdditionalFiles.insert({File.first, File.second});
+ auto AST = TU.build();
std::vector<HighlightingToken> ActualTokens = getSemanticHighlightings(AST);
EXPECT_THAT(ActualTokens, getExpectedTokens(Test)) << Code;
}
@@ -321,6 +328,17 @@
for (const auto &TestCase : TestCases) {
checkHighlightings(TestCase);
}
+
+ checkHighlightings(R"cpp(
+ class $Class[[A]] {
+ #include "imp.h"
+ };
+ #endif
+ )cpp",
+ {{"imp.h", R"cpp(
+ int someMethod();
+ void otherMethod();
+ )cpp"}});
}
TEST(SemanticHighlighting, GeneratesHighlightsWhenFileChange) {
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===================================================================
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -231,6 +231,12 @@
// FIXME: skip tokens inside macros for now.
return;
+ // Non top level decls that are included from a header are not filtered by
+ // topLevelDecls. (example: method declarations being included from another
+ // file for a class) from another file)
+ if (!SM.isWrittenInMainFile(Loc))
+ return;
+
auto R = getTokenRange(SM, Ctx.getLangOpts(), Loc);
if (!R) {
// R should always have a value, if it doesn't something is very wrong.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
