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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
