[PATCH] D63821: [clangd] Added C++ API code for semantic highlighting
This revision was automatically updated to reflect the committed changes.
Closed by commit rL364551: [clangd] Emit semantic highlighting tokens when the
main AST is built. (authored by jvikstrom, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D63821?vs=206854=206867#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63821/new/
https://reviews.llvm.org/D63821
Files:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
Index: clang-tools-extra/trunk/clangd/ClangdServer.h
===
--- clang-tools-extra/trunk/clangd/ClangdServer.h
+++ clang-tools-extra/trunk/clangd/ClangdServer.h
@@ -18,6 +18,7 @@
#include "Function.h"
#include "GlobalCompilationDatabase.h"
#include "Protocol.h"
+#include "SemanticHighlighting.h"
#include "TUScheduler.h"
#include "XRefs.h"
#include "index/Background.h"
@@ -49,6 +50,11 @@
std::vector Diagnostics) = 0;
/// Called whenever the file status is updated.
virtual void onFileUpdated(PathRef File, const TUStatus ){};
+
+ /// Called by ClangdServer when some \p Highlightings for \p File are ready.
+ virtual void
+ onHighlightingsReady(PathRef File,
+ std::vector Highlightings) {}
};
/// When set, used by ClangdServer to get clang-tidy options for each particular
@@ -131,6 +137,9 @@
/// Clangd will execute compiler drivers matching one of these globs to
/// fetch system include path.
std::vector QueryDriverGlobs;
+
+/// Enable semantic highlighting features.
+bool SemanticHighlighting = false;
};
// Sensible default options for use in tests.
// Features like indexing must be enabled if desired.
@@ -304,7 +313,7 @@
// can be caused by missing includes (e.g. member access in incomplete type).
bool SuggestMissingIncludes = false;
bool EnableHiddenFeatures = false;
-
+
// GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
llvm::StringMap>
CachedCompletionFuzzyFindRequestByFile;
Index: clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
@@ -7,7 +7,9 @@
//===--===//
#include "Annotations.h"
+#include "ClangdServer.h"
#include "SemanticHighlighting.h"
+#include "TestFS.h"
#include "TestTU.h"
#include "gmock/gmock.h"
@@ -64,6 +66,30 @@
}
}
+TEST(ClangdSemanticHighlightingTest, GeneratesHighlightsWhenFileChange) {
+ class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {
+ public:
+std::atomic Count = {0};
+
+void onDiagnosticsReady(PathRef, std::vector) override {}
+void onHighlightingsReady(
+PathRef File, std::vector Highlightings) override {
+ ++Count;
+}
+ };
+
+ auto FooCpp = testPath("foo.cpp");
+ MockFSProvider FS;
+ FS.Files[FooCpp] = "";
+
+ MockCompilationDatabase MCD;
+ HighlightingsCounterDiagConsumer DiagConsumer;
+ ClangdServer Server(MCD, FS, DiagConsumer, ClangdServer::optsForTest());
+ Server.addDocument(FooCpp, "int a;");
+ ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for server";
+ ASSERT_EQ(DiagConsumer.Count, 1);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp
@@ -48,8 +48,10 @@
// Update the FileIndex with new ASTs and plumb the diagnostics responses.
struct UpdateIndexCallbacks : public ParsingCallbacks {
- UpdateIndexCallbacks(FileIndex *FIndex, DiagnosticsConsumer )
- : FIndex(FIndex), DiagConsumer(DiagConsumer) {}
+ UpdateIndexCallbacks(FileIndex *FIndex, DiagnosticsConsumer ,
+ bool SemanticHighlighting)
+ : FIndex(FIndex), DiagConsumer(DiagConsumer),
+SemanticHighlighting(SemanticHighlighting) {}
void onPreambleAST(PathRef Path, ASTContext ,
std::shared_ptr PP,
@@ -61,6 +63,8 @@
void onMainAST(PathRef Path, ParsedAST ) override {
if (FIndex)
FIndex->updateMain(Path, AST);
+if (SemanticHighlighting)
+ DiagConsumer.onHighlightingsReady(Path, getSemanticHighlightings(AST));
}
void onDiagnostics(PathRef File, std::vector Diags) override {
@@ -74,6 +78,7 @@
private:
FileIndex *FIndex;
DiagnosticsConsumer
+ bool SemanticHighlighting;
};
} // namespace
@@ -82,6 +87,7 @@
Opts.UpdateDebounce =
[PATCH] D63821: [clangd] Added C++ API code for semantic highlighting
hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
Let's rephrase the commit message, I think this patch is just to "emit the
semantic highlighting tokens when the main AST is built".
Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:844
+TEST(ClangdSemanticHighlightingTest, GeneratesHighlightsWhenFileChange) {
+ class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {
maybe move this test to SemanticHighlightingTests.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63821/new/
https://reviews.llvm.org/D63821
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63821: [clangd] Added C++ API code for semantic highlighting
jvikstrom updated this revision to Diff 206854.
jvikstrom marked 2 inline comments as done.
jvikstrom added a comment.
Made test safe again
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63821/new/
https://reviews.llvm.org/D63821
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.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
@@ -841,6 +841,30 @@
EXPECT_FALSE(PathResult.hasValue());
}
+TEST(ClangdSemanticHighlightingTest, GeneratesHighlightsWhenFileChange) {
+ class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {
+ public:
+std::atomic Count = {0};
+
+void onDiagnosticsReady(PathRef, std::vector) override {}
+void onHighlightingsReady(
+PathRef File, std::vector Highlightings) override {
+ ++Count;
+}
+ };
+
+ auto FooCpp = testPath("foo.cpp");
+ MockFSProvider FS;
+ FS.Files[FooCpp] = "";
+
+ MockCompilationDatabase MCD;
+ HighlightingsCounterDiagConsumer DiagConsumer;
+ ClangdServer Server(MCD, FS, DiagConsumer, ClangdServer::optsForTest());
+ Server.addDocument(FooCpp, "int a;");
+ ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for server";
+ ASSERT_EQ(DiagConsumer.Count, 1);
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -18,6 +18,7 @@
#include "Function.h"
#include "GlobalCompilationDatabase.h"
#include "Protocol.h"
+#include "SemanticHighlighting.h"
#include "TUScheduler.h"
#include "XRefs.h"
#include "index/Background.h"
@@ -49,6 +50,11 @@
std::vector Diagnostics) = 0;
/// Called whenever the file status is updated.
virtual void onFileUpdated(PathRef File, const TUStatus ){};
+
+ /// Called by ClangdServer when some \p Highlightings for \p File are ready.
+ virtual void
+ onHighlightingsReady(PathRef File,
+ std::vector Highlightings) {}
};
/// When set, used by ClangdServer to get clang-tidy options for each particular
@@ -131,6 +137,9 @@
/// Clangd will execute compiler drivers matching one of these globs to
/// fetch system include path.
std::vector QueryDriverGlobs;
+
+/// Enable semantic highlighting features.
+bool SemanticHighlighting = false;
};
// Sensible default options for use in tests.
// Features like indexing must be enabled if desired.
@@ -304,7 +313,7 @@
// can be caused by missing includes (e.g. member access in incomplete type).
bool SuggestMissingIncludes = false;
bool EnableHiddenFeatures = false;
-
+
// GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
llvm::StringMap>
CachedCompletionFuzzyFindRequestByFile;
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -48,8 +48,10 @@
// Update the FileIndex with new ASTs and plumb the diagnostics responses.
struct UpdateIndexCallbacks : public ParsingCallbacks {
- UpdateIndexCallbacks(FileIndex *FIndex, DiagnosticsConsumer )
- : FIndex(FIndex), DiagConsumer(DiagConsumer) {}
+ UpdateIndexCallbacks(FileIndex *FIndex, DiagnosticsConsumer ,
+ bool SemanticHighlighting)
+ : FIndex(FIndex), DiagConsumer(DiagConsumer),
+SemanticHighlighting(SemanticHighlighting) {}
void onPreambleAST(PathRef Path, ASTContext ,
std::shared_ptr PP,
@@ -61,6 +63,8 @@
void onMainAST(PathRef Path, ParsedAST ) override {
if (FIndex)
FIndex->updateMain(Path, AST);
+if (SemanticHighlighting)
+ DiagConsumer.onHighlightingsReady(Path, getSemanticHighlightings(AST));
}
void onDiagnostics(PathRef File, std::vector Diags) override {
@@ -74,6 +78,7 @@
private:
FileIndex *FIndex;
DiagnosticsConsumer
+ bool SemanticHighlighting;
};
} // namespace
@@ -82,6 +87,7 @@
Opts.UpdateDebounce = std::chrono::steady_clock::duration::zero(); // Faster!
Opts.StorePreamblesInMemory = true;
Opts.AsyncThreadsCount = 4; // Consistent!
+ Opts.SemanticHighlighting = true;
return Opts;
}
@@ -102,10 +108,11 @@
// is parsed.
// FIXME(ioeric): this can be slow and we may be able to index on less
// critical paths.
- WorkScheduler(CDB, Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
-llvm::make_unique(DynamicIdx.get(),
-
[PATCH] D63821: [clangd] Added C++ API code for semantic highlighting
hokein added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:60 + void + onHighlightingsReady(PathRef File, + std::vector Highlightings) override; jvikstrom wrote: > hokein wrote: > > nit: you can remove this override, since we have provided an empty default > > implementation. > I get an `-Winconsistent-missing-override` warnings without the override. sorry for the confusion, I meant we remove `onHighlightingsReady`, not the `override` keyword. Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:864 + ClangdServer Server(MCD, FS, DiagConsumer, ClangdServer::optsForTest()); + Server.addDocument(FooCpp, "int a;"); + ASSERT_EQ(DiagConsumer.Count, 1); this is not safe, we need to wait until the server finishes building AST. add `ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for server"; ` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63821/new/ https://reviews.llvm.org/D63821 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63821: [clangd] Added C++ API code for semantic highlighting
jvikstrom marked an inline comment as done. jvikstrom added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:60 + void + onHighlightingsReady(PathRef File, + std::vector Highlightings) override; hokein wrote: > nit: you can remove this override, since we have provided an empty default > implementation. I get an `-Winconsistent-missing-override` warnings without the override. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63821/new/ https://reviews.llvm.org/D63821 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63821: [clangd] Added C++ API code for semantic highlighting
jvikstrom updated this revision to Diff 206841.
jvikstrom marked an inline comment as done.
jvikstrom added a comment.
Simplified test.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63821/new/
https://reviews.llvm.org/D63821
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.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
@@ -13,6 +13,7 @@
#include "GlobalCompilationDatabase.h"
#include "Matchers.h"
#include "SyncAPI.h"
+#include "TUScheduler.h"
#include "TestFS.h"
#include "Threading.h"
#include "URI.h"
@@ -841,6 +842,29 @@
EXPECT_FALSE(PathResult.hasValue());
}
+TEST(ClangdSemanticHighlightingTest, GeneratesHighlightsWhenFileChange) {
+ class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {
+ public:
+std::atomic Count = {0};
+
+void onDiagnosticsReady(PathRef, std::vector) override {}
+void onHighlightingsReady(
+PathRef File, std::vector Highlightings) override {
+ ++Count;
+}
+ };
+
+ auto FooCpp = testPath("foo.cpp");
+ MockFSProvider FS;
+ FS.Files[FooCpp] = "";
+
+ MockCompilationDatabase MCD;
+ HighlightingsCounterDiagConsumer DiagConsumer;
+ ClangdServer Server(MCD, FS, DiagConsumer, ClangdServer::optsForTest());
+ Server.addDocument(FooCpp, "int a;");
+ ASSERT_EQ(DiagConsumer.Count, 1);
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -18,6 +18,7 @@
#include "Function.h"
#include "GlobalCompilationDatabase.h"
#include "Protocol.h"
+#include "SemanticHighlighting.h"
#include "TUScheduler.h"
#include "XRefs.h"
#include "index/Background.h"
@@ -49,6 +50,11 @@
std::vector Diagnostics) = 0;
/// Called whenever the file status is updated.
virtual void onFileUpdated(PathRef File, const TUStatus ){};
+
+ /// Called by ClangdServer when some \p Highlightings for \p File are ready.
+ virtual void
+ onHighlightingsReady(PathRef File,
+ std::vector Highlightings) {}
};
/// When set, used by ClangdServer to get clang-tidy options for each particular
@@ -131,6 +137,9 @@
/// Clangd will execute compiler drivers matching one of these globs to
/// fetch system include path.
std::vector QueryDriverGlobs;
+
+/// Enable semantic highlighting features.
+bool SemanticHighlighting = false;
};
// Sensible default options for use in tests.
// Features like indexing must be enabled if desired.
@@ -304,7 +313,7 @@
// can be caused by missing includes (e.g. member access in incomplete type).
bool SuggestMissingIncludes = false;
bool EnableHiddenFeatures = false;
-
+
// GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
llvm::StringMap>
CachedCompletionFuzzyFindRequestByFile;
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -48,8 +48,10 @@
// Update the FileIndex with new ASTs and plumb the diagnostics responses.
struct UpdateIndexCallbacks : public ParsingCallbacks {
- UpdateIndexCallbacks(FileIndex *FIndex, DiagnosticsConsumer )
- : FIndex(FIndex), DiagConsumer(DiagConsumer) {}
+ UpdateIndexCallbacks(FileIndex *FIndex, DiagnosticsConsumer ,
+ bool SemanticHighlighting)
+ : FIndex(FIndex), DiagConsumer(DiagConsumer),
+SemanticHighlighting(SemanticHighlighting) {}
void onPreambleAST(PathRef Path, ASTContext ,
std::shared_ptr PP,
@@ -61,6 +63,8 @@
void onMainAST(PathRef Path, ParsedAST ) override {
if (FIndex)
FIndex->updateMain(Path, AST);
+if (SemanticHighlighting)
+ DiagConsumer.onHighlightingsReady(Path, getSemanticHighlightings(AST));
}
void onDiagnostics(PathRef File, std::vector Diags) override {
@@ -74,6 +78,7 @@
private:
FileIndex *FIndex;
DiagnosticsConsumer
+ bool SemanticHighlighting;
};
} // namespace
@@ -82,6 +87,7 @@
Opts.UpdateDebounce = std::chrono::steady_clock::duration::zero(); // Faster!
Opts.StorePreamblesInMemory = true;
Opts.AsyncThreadsCount = 4; // Consistent!
+ Opts.SemanticHighlighting = true;
return Opts;
}
@@ -102,10 +108,11 @@
// is parsed.
// FIXME(ioeric): this can be slow and
[PATCH] D63821: [clangd] Added C++ API code for semantic highlighting
hokein added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:60 + void + onHighlightingsReady(PathRef File, + std::vector Highlightings) override; nit: you can remove this override, since we have provided an empty default implementation. Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:881 + + std::promise StartSecondPromise; + std::future StartSecond = StartSecondPromise.get_future(); Looks like the current test could be simplified, I think we want to test that when the AST is build/rebuild, we will emit the highlighting tokens. Just use `Count` and check the count is to 1? ``` Server.addDocument(FooCpp, "int a;"); ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for server"; ASSERT_EQ(DiagConsumer.Count, 1); ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63821/new/ https://reviews.llvm.org/D63821 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63821: [clangd] Added C++ API code for semantic highlighting
jvikstrom updated this revision to Diff 206832.
jvikstrom marked 9 inline comments as done.
jvikstrom added a comment.
Separated test and gave consumer an empty definition.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63821/new/
https://reviews.llvm.org/D63821
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.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
@@ -841,6 +841,56 @@
EXPECT_FALSE(PathResult.hasValue());
}
+TEST(ClangdSemanticHighlightingTest, GeneratesHighlightsWhenFileChange) {
+ class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {
+ public:
+std::atomic Count = {0};
+
+HighlightingsCounterDiagConsumer(std::promise StartSecondReparse)
+: StartSecondReparse(std::move(StartSecondReparse)) {}
+
+void onDiagnosticsReady(PathRef, std::vector) override {}
+
+virtual void onHighlightingsReady(
+PathRef File, std::vector Highlightings) override {
+ ++Count;
+ if (FirstRequest) {
+FirstRequest = false;
+StartSecondReparse.set_value();
+ }
+}
+
+ private:
+std::mutex Mutex;
+bool FirstRequest = true;
+std::promise StartSecondReparse;
+ };
+
+ const auto SourceContents1 = R"cpp(
+int a;
+)cpp";
+
+ const auto SourceContents2 = R"cpp(
+int a = ;
+)cpp";
+
+ auto FooCpp = testPath("foo.cpp");
+ MockFSProvider FS;
+ FS.Files[FooCpp] = "";
+
+ std::promise StartSecondPromise;
+ std::future StartSecond = StartSecondPromise.get_future();
+ MockCompilationDatabase MCD;
+ HighlightingsCounterDiagConsumer DiagConsumer(std::move(StartSecondPromise));
+ ClangdServer Server(MCD, FS, DiagConsumer,
+ ClangdServer::optsForTest());
+ Server.addDocument(FooCpp, SourceContents1);
+ StartSecond.wait();
+ Server.addDocument(FooCpp, SourceContents2);
+ ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for server";
+ ASSERT_EQ(DiagConsumer.Count, 2);
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -18,6 +18,7 @@
#include "Function.h"
#include "GlobalCompilationDatabase.h"
#include "Protocol.h"
+#include "SemanticHighlighting.h"
#include "TUScheduler.h"
#include "XRefs.h"
#include "index/Background.h"
@@ -49,6 +50,11 @@
std::vector Diagnostics) = 0;
/// Called whenever the file status is updated.
virtual void onFileUpdated(PathRef File, const TUStatus ){};
+
+ /// Called by ClangdServer when some \p Highlightings for \p File are ready.
+ virtual void
+ onHighlightingsReady(PathRef File,
+ std::vector Highlightings) {}
};
/// When set, used by ClangdServer to get clang-tidy options for each particular
@@ -131,6 +137,9 @@
/// Clangd will execute compiler drivers matching one of these globs to
/// fetch system include path.
std::vector QueryDriverGlobs;
+
+/// Enable semantic highlighting features.
+bool SemanticHighlighting = false;
};
// Sensible default options for use in tests.
// Features like indexing must be enabled if desired.
@@ -304,7 +313,7 @@
// can be caused by missing includes (e.g. member access in incomplete type).
bool SuggestMissingIncludes = false;
bool EnableHiddenFeatures = false;
-
+
// GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
llvm::StringMap>
CachedCompletionFuzzyFindRequestByFile;
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -48,8 +48,10 @@
// Update the FileIndex with new ASTs and plumb the diagnostics responses.
struct UpdateIndexCallbacks : public ParsingCallbacks {
- UpdateIndexCallbacks(FileIndex *FIndex, DiagnosticsConsumer )
- : FIndex(FIndex), DiagConsumer(DiagConsumer) {}
+ UpdateIndexCallbacks(FileIndex *FIndex, DiagnosticsConsumer ,
+ bool SemanticHighlighting)
+ : FIndex(FIndex), DiagConsumer(DiagConsumer),
+SemanticHighlighting(SemanticHighlighting) {}
void onPreambleAST(PathRef Path, ASTContext ,
std::shared_ptr PP,
@@ -61,6 +63,8 @@
void onMainAST(PathRef Path, ParsedAST ) override {
if (FIndex)
FIndex->updateMain(Path, AST);
+if
[PATCH] D63821: [clangd] Added C++ API code for semantic highlighting
hokein added a comment.
thanks, it is much clearer now.
Could you please make the commit message be more specific what this patch does?
C++ APIs is too generous, (we already have C++ APIs and data structures for
semantic highlightings which are in `SemanticHighlighting.h`).
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:60
+ void onHighlightingsReady(PathRef File,
+ std::vector Highlightings)
override;
clang-format: intention is not correct.
Comment at: clang-tools-extra/clangd/ClangdServer.h:60
+ // Called by ClangdServer when some \p Highlightings for \p File are ready.
+ virtual void onHighlightingsReady(PathRef File,
+ std::vector Highlightings)
= 0;
nik wrote:
> jvikstrom wrote:
> > hokein wrote:
> > > we may add this interface to the existing `DiagnosticsConsumer`.
> > Probably want to rename `DiagnosticsConsumer` as well, can't come up with a
> > good name though. Any suggestions?
> One could summarize diagnostics and highlightings as annotations, so maybe
> FileAnnotationsConsumer or DocumentAnnotationsConsumer? Not sure how
> onFileUpdated() fits into this...
There is a FIXME on Line 43. I'd keep it unchanged now (I don't have a good
name either).
Comment at: clang-tools-extra/clangd/ClangdServer.h:54
+
+ // Called by ClangdServer when some \p Highlightings for \p File are ready.
+ virtual void onHighlightingsReady(PathRef File,
nit: triple `/` here.
Comment at: clang-tools-extra/clangd/ClangdServer.h:56
+ virtual void onHighlightingsReady(PathRef File,
+ std::vector Highlightings)
= 0;
};
just provide an empty implementation (like `onFileUpdated`), and you don't need
to update all the subclasses.
Comment at: clang-tools-extra/clangd/ClangdServer.h:140
+
+// If true Clangd will generate semantic highlightings for the current
+// document when it changes.
just: `Enable semantic highlighting features`, and name it `bool
SemanticHighlighting`.
Comment at: clang-tools-extra/clangd/ClangdServer.h:160
/// synchronize access to shared state.
+ /// If semantic highlighting is enabled ClangdServer also generates semantic
+ /// highlightings for the file after each parsing request. When highlightings
not sure whether we need this comment, because the intention is not obvious in
code here (the flag is hidden in Opts), it might be more sensible to move it to
`Options::SemanticHighlighting`.
And the comment is not precise in the new code now (we collect the highlighting
tokens in the DiagConsumer now).
Comment at: clang-tools-extra/clangd/ClangdServer.h:320
+
+ // If this is true semantic highlighting will be generated.
+ bool SemanticHighlightingEnabled = false;
We don't need to store it as a class member, we only pass this value to
`ParsingCallBack` in the constructor.
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:35
+// scopes.
+std::map>
+getSemanticHighlightingScopes();
I think it is unrelated to this patch, we should include this when we
implements the LSP part.
Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:863
std::atomic Count = {0};
+std::atomic HighlightingCount = {0};
We should keep the test isolated, this test is for diagnostics, please add a
new `TEST_F` for merely testing the highlightings.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63821/new/
https://reviews.llvm.org/D63821
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63821: [clangd] Added C++ API code for semantic highlighting
nik added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.h:60 + // Called by ClangdServer when some \p Highlightings for \p File are ready. + virtual void onHighlightingsReady(PathRef File, + std::vector Highlightings) = 0; jvikstrom wrote: > hokein wrote: > > we may add this interface to the existing `DiagnosticsConsumer`. > Probably want to rename `DiagnosticsConsumer` as well, can't come up with a > good name though. Any suggestions? One could summarize diagnostics and highlightings as annotations, so maybe FileAnnotationsConsumer or DocumentAnnotationsConsumer? Not sure how onFileUpdated() fits into this... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63821/new/ https://reviews.llvm.org/D63821 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63821: [clangd] Added C++ API code for semantic highlighting
jvikstrom marked 2 inline comments as done. jvikstrom added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.h:60 + // Called by ClangdServer when some \p Highlightings for \p File are ready. + virtual void onHighlightingsReady(PathRef File, + std::vector Highlightings) = 0; hokein wrote: > we may add this interface to the existing `DiagnosticsConsumer`. Probably want to rename `DiagnosticsConsumer` as well, can't come up with a good name though. Any suggestions? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63821/new/ https://reviews.llvm.org/D63821 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63821: [clangd] Added C++ API code for semantic highlighting
jvikstrom updated this revision to Diff 206792.
jvikstrom added a comment.
Herald added a subscriber: jfb.
Moved semantic highlighting to be processed in onMainAST
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63821/new/
https://reviews.llvm.org/D63821
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/SemanticHighlighting.cpp
clang-tools-extra/clangd/SemanticHighlighting.h
clang-tools-extra/clangd/unittests/ClangdTests.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
clang-tools-extra/clangd/unittests/XRefsTests.cpp
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -36,6 +36,10 @@
class IgnoreDiagnostics : public DiagnosticsConsumer {
void onDiagnosticsReady(PathRef File,
std::vector Diagnostics) override {}
+
+ virtual void
+ onHighlightingsReady(PathRef File,
+std::vector Highlightings) override {}
};
MATCHER_P2(FileRange, File, Range, "") {
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -677,6 +677,9 @@
AllStatus.push_back(Status);
}
+virtual void onHighlightingsReady(
+PathRef File, std::vector Highlightings) override {}
+
std::vector allStatus() {
std::lock_guard Lock(Mutex);
return AllStatus;
Index: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
+++ clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
@@ -29,6 +29,9 @@
class IgnoreDiagnostics : public DiagnosticsConsumer {
void onDiagnosticsReady(PathRef File,
std::vector Diagnostics) override {}
+ virtual void
+ onHighlightingsReady(PathRef File,
+std::vector Highlightings) override {}
};
// GMock helpers for matching SymbolInfos items.
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -45,6 +45,9 @@
class IgnoreDiagnostics : public DiagnosticsConsumer {
void onDiagnosticsReady(PathRef File,
std::vector Diagnostics) override {}
+ virtual void
+ onHighlightingsReady(PathRef File,
+std::vector Highlightings) override {}
};
// GMock helpers for matching completion items.
Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -73,6 +73,10 @@
return HadErrorInLastDiags;
}
+ virtual void
+ onHighlightingsReady(PathRef File,
+std::vector Highlightings) override {}
+
private:
std::mutex Mutex;
bool HadErrorInLastDiags = false;
@@ -101,6 +105,10 @@
return Result;
}
+ virtual void
+ onHighlightingsReady(PathRef File,
+std::vector Highlightings) override {}
+
void clear() {
std::lock_guard Lock(Mutex);
LastDiagsHadError.clear();
@@ -279,6 +287,9 @@
std::vector Diagnostics) override {
Got = Context::current().getExisting(Secret);
}
+ virtual void
+ onHighlightingsReady(PathRef File,
+std::vector Highlightings) override {}
int Got;
} DiagConsumer;
MockCompilationDatabase CDB;
@@ -610,6 +621,10 @@
Stats[FileIndex].HadErrorsInLastDiags = HadError;
}
+virtual void
+onHighlightingsReady(PathRef File,
+ std::vector Highlightings) override {}
+
std::vector takeFileStats() {
std::lock_guard Lock(Mutex);
return std::move(Stats);
@@ -845,6 +860,7 @@
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
std::atomic Count = {0};
+std::atomic HighlightingCount = {0};
NoConcurrentAccessDiagConsumer(std::promise StartSecondReparse)
: StartSecondReparse(std::move(StartSecondReparse)) {}
@@ -865,6 +881,11 @@
}
}
+virtual void onHighlightingsReady(
+PathRef File, std::vector Highlightings) override {
+
[PATCH] D63821: [clangd] Added C++ API code for semantic highlighting
jvikstrom created this revision.
jvikstrom added reviewers: hokein, ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay,
javed.absar.
Herald added a project: clang.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D63821
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/SemanticHighlighting.cpp
clang-tools-extra/clangd/SemanticHighlighting.h
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
clang-tools-extra/clangd/unittests/ClangdTests.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
clang-tools-extra/clangd/unittests/XRefsTests.cpp
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -10,6 +10,7 @@
#include "Compiler.h"
#include "Matchers.h"
#include "Protocol.h"
+#include "SemanticHighlighting.h"
#include "SyncAPI.h"
#include "TestFS.h"
#include "TestTU.h"
@@ -33,9 +34,13 @@
using ::testing::Matcher;
using ::testing::UnorderedElementsAreArray;
-class IgnoreDiagnostics : public DiagnosticsConsumer {
+class IgnoreDiagnostics : public DiagnosticsConsumer, public HighlightingsConsumer {
void onDiagnosticsReady(PathRef File,
std::vector Diagnostics) override {}
+
+ virtual void
+ onHighlightingsReady(PathRef File,
+std::vector Highlightings) override {}
};
MATCHER_P2(FileRange, File, Range, "") {
@@ -539,7 +544,7 @@
IgnoreDiagnostics DiagConsumer;
MockFSProvider FS;
- ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+ ClangdServer Server(CDB, FS, DiagConsumer, DiagConsumer, ClangdServer::optsForTest());
// Fill the filesystem.
auto FooCpp = testPath("src/foo.cpp");
@@ -1791,7 +1796,7 @@
MockFSProvider FS;
IgnoreDiagnostics DiagConsumer;
MockCompilationDatabase CDB;
- ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+ ClangdServer Server(CDB, FS, DiagConsumer, DiagConsumer, ClangdServer::optsForTest());
auto FooCpp = testPath("foo.cpp");
const char *SourceContents = R"cpp(
@@ -1866,7 +1871,7 @@
MockFSProvider FS;
IgnoreDiagnostics DiagConsumer;
MockCompilationDatabase CDB;
- ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+ ClangdServer Server(CDB, FS, DiagConsumer, DiagConsumer, ClangdServer::optsForTest());
auto FooCpp = testPath("foo.cpp");
// The trigger locations must be the same.
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -7,6 +7,7 @@
//===--===//
#include "Annotations.h"
+#include "ClangdServer.h"
#include "Context.h"
#include "Matchers.h"
#include "TUScheduler.h"
@@ -44,9 +45,10 @@
void updateWithCallback(TUScheduler , PathRef File,
llvm::StringRef Contents, WantDiagnostics WD,
+ bool WantHighlight,
llvm::unique_function CB) {
WithContextValue Ctx(llvm::make_scope_exit(std::move(CB)));
-S.update(File, getInputs(File, Contents), WD);
+S.update(File, getInputs(File, Contents), WD, WantHighlight);
}
static Key)>>
@@ -71,7 +73,7 @@
/// any. The TUScheduler should be created with captureDiags as a
/// DiagsCallback for this to work.
void updateWithDiags(TUScheduler , PathRef File, ParseInputs Inputs,
- WantDiagnostics WD,
+ WantDiagnostics WD, bool WantHighlight,
llvm::unique_function)> CB) {
Path OrigFile = File.str();
WithContextValue Ctx(
@@ -82,13 +84,13 @@
CB(std::move(Diags));
},
std::move(CB)));
-S.update(File, std::move(Inputs), WD);
+S.update(File, std::move(Inputs), WD, WantHighlight);
}
void updateWithDiags(TUScheduler , PathRef File, llvm::StringRef Contents,
- WantDiagnostics WD,
+ WantDiagnostics WD, bool WantHighlight,
llvm::unique_function)> CB) {
-return updateWithDiags(S, File, getInputs(File, Contents), WD,
+return updateWithDiags(S, File, getInputs(File, Contents), WD, WantHighlight,
std::move(CB));
}
@@ -113,7 +115,7 @@
Files[Missing] = "";
EXPECT_EQ(S.getContents(Added), "");
-
