This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Added C++ API code for semantic highlighting
ClosedPublic

Authored by jvikstrom on Jun 26 2019, 7:12 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

jvikstrom created this revision.Jun 26 2019, 7:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2019, 7:12 AM
ilya-biryukov added inline comments.Jun 26 2019, 7:58 AM
clang-tools-extra/clangd/TUScheduler.h
157 ↗(On Diff #206661)

Can we avoid updating TUScheduler, ParsingCallbacks and compute the highlights inside onMainAST?

hokein added inline comments.Jun 26 2019, 8:07 AM
clang-tools-extra/clangd/ClangdServer.h
60 ↗(On Diff #206661)

we may add this interface to the existing DiagnosticsConsumer.

clang-tools-extra/clangd/TUScheduler.h
157 ↗(On Diff #206661)

+1.

jvikstrom updated this revision to Diff 206792.Jun 27 2019, 1:17 AM

Moved semantic highlighting to be processed in onMainAST

jvikstrom marked 2 inline comments as done.Jun 27 2019, 1:19 AM
jvikstrom added inline comments.
clang-tools-extra/clangd/ClangdServer.h
60 ↗(On Diff #206661)

Probably want to rename DiagnosticsConsumer as well, can't come up with a good name though. Any suggestions?

jvikstrom marked 3 inline comments as done.Jun 27 2019, 1:19 AM
nik added a subscriber: nik.Jun 27 2019, 1:42 AM
nik added inline comments.
clang-tools-extra/clangd/ClangdServer.h
60 ↗(On Diff #206661)

One could summarize diagnostics and highlightings as annotations, so maybe FileAnnotationsConsumer or DocumentAnnotationsConsumer? Not sure how onFileUpdated() fits into this...

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).

clang-tools-extra/clangd/ClangdLSPServer.h
60 ↗(On Diff #206792)

clang-format: intention is not correct.

clang-tools-extra/clangd/ClangdServer.h
54 ↗(On Diff #206792)

nit: triple / here.

56 ↗(On Diff #206792)

just provide an empty implementation (like onFileUpdated), and you don't need to update all the subclasses.

140 ↗(On Diff #206792)

just: Enable semantic highlighting features, and name it bool SemanticHighlighting.

160 ↗(On Diff #206792)

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).

320 ↗(On Diff #206792)

We don't need to store it as a class member, we only pass this value to ParsingCallBack in the constructor.

60 ↗(On Diff #206661)

There is a FIXME on Line 43. I'd keep it unchanged now (I don't have a good name either).

clang-tools-extra/clangd/SemanticHighlighting.h
35 ↗(On Diff #206792)

I think it is unrelated to this patch, we should include this when we implements the LSP part.

clang-tools-extra/clangd/unittests/ClangdTests.cpp
863 ↗(On Diff #206792)

We should keep the test isolated, this test is for diagnostics, please add a new TEST_F for merely testing the highlightings.

jvikstrom updated this revision to Diff 206832.Jun 27 2019, 4:55 AM
jvikstrom marked 9 inline comments as done.

Separated test and gave consumer an empty definition.

hokein added inline comments.Jun 27 2019, 5:30 AM
clang-tools-extra/clangd/ClangdLSPServer.h
60 ↗(On Diff #206832)

nit: you can remove this override, since we have provided an empty default implementation.

clang-tools-extra/clangd/unittests/ClangdTests.cpp
881 ↗(On Diff #206832)

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);
jvikstrom marked an inline comment as done.Jun 27 2019, 5:54 AM
jvikstrom added inline comments.
clang-tools-extra/clangd/ClangdLSPServer.h
60 ↗(On Diff #206832)

I get an -Winconsistent-missing-override warnings without the override.

jvikstrom updated this revision to Diff 206841.Jun 27 2019, 5:56 AM
jvikstrom marked an inline comment as done.

Simplified test.

hokein added inline comments.Jun 27 2019, 6:19 AM
clang-tools-extra/clangd/ClangdLSPServer.h
60 ↗(On Diff #206832)

sorry for the confusion, I meant we remove onHighlightingsReady, not the override keyword.

clang-tools-extra/clangd/unittests/ClangdTests.cpp
864 ↗(On Diff #206841)

this is not safe, we need to wait until the server finishes building AST.

add ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for server";

jvikstrom updated this revision to Diff 206854.Jun 27 2019, 6:50 AM
jvikstrom marked 2 inline comments as done.

Made test safe again

hokein accepted this revision.Jun 27 2019, 7:50 AM

Let's rephrase the commit message, I think this patch is just to "emit the semantic highlighting tokens when the main AST is built".

clang-tools-extra/clangd/unittests/ClangdTests.cpp
844 ↗(On Diff #206854)

maybe move this test to SemanticHighlightingTests.

This revision is now accepted and ready to land.Jun 27 2019, 7:50 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2019, 8:17 AM