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<HighlightingToken> 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<HighlightingToken> 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<HighlightingToken> 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<HighlightingKind, std::vector<std::string>>
+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<int> Count = {0};
+ std::atomic<int> 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
