[PATCH] D63559: [clangd] Added functionality for getting semantic highlights for variable and function declarations

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:28
+      : Ctx(AST.getASTContext()), SM(AST.getSourceManager()) {
+    Ctx.setTraversalScope(AST.getLocalTopLevelDecls());
+  }
----------------
hokein wrote:
> I'd move this line to `collectTokens` as they are related.
> 
> As discussed, setTraversalScope doesn't guarantee that we wouldnot visit Decl* outside of the main file (some decls are still reachable), so we may get tokens outside of the main file. If you don't address it in this patch, that's fine, leave a FIXME here.
Will fix in a separate patch for topLevelDecls. Don't really know what FIXME to add  here. Added one to getLocalTopLevelDecls. Don't really have a good understanding of the reason as to what happens yet so the FIXME is very general (as you can probably tell from it)...


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:56
+
+    auto R = getTokenRange(SM, Ctx.getLangOpts(), D->getLocation());
+    if (!R.hasValue()) {
----------------
hokein wrote:
> How about pulling out a function `llvm::Optional<SemanticToken> makeSemanticToken(..)`?
Don't understand what you mean.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:24
+
+std::vector<SemanticToken> makeSemanticTokens(std::vector<Range> Ranges,
+                                             SemanticHighlightKind Kind) {
----------------
hokein wrote:
> we are passing a copy here, use llvm::ArrayRef.
I changed to pass a const vector & instead. Is that alright as well?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63559/new/

https://reviews.llvm.org/D63559