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?

  rG LLVM Github Monorepo



cfe-commits mailing list

Reply via email to