[PATCH] D63821: [clangd] Added C++ API code for semantic highlighting

Haojian Wu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jun 27 02:00:37 PDT 2019


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





More information about the cfe-commits mailing list