hokein added a comment.
much clearer now, a few more nits.
================
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:69
bool HandleTopLevelDecl(DeclGroupRef DG) override {
+ //FIXME: There is an edge case where this will still get decls from
include files.
for (Decl *D : DG) {
----------------
not need to add this in this patch as you are also working on a fix in a
separate patch.
================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:56
+
+ auto R = getTokenRange(SM, Ctx.getLangOpts(), D->getLocation());
+ if (!R.hasValue()) {
----------------
jvikstrom wrote:
> hokein wrote:
> > How about pulling out a function `llvm::Optional<SemanticToken>
> > makeSemanticToken(..)`?
> Don't understand what you mean.
I meant we could move lines 56 ~ 66 to a new function, the current
implementation is fine (as `addToken` is small).
================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:51
+
+ if (D->getDeclName().isEmpty()) {
+ // Don't add symbols that don't have any length.
----------------
nit: remove the `{}` if the body contains only one statement, the same to other
places.
================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:57
+ auto R = getTokenRange(SM, Ctx.getLangOpts(), D->getLocation());
+ if (!R.hasValue()) {
+ // R should always have a value, if it doesn't something is very wrong.
----------------
nit: just "if (!R)" would work.
================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:64
+ SemanticToken S;
+ S.R = R.getValue();
+ S.Kind = Kind;
----------------
maybe just `Tokens.emplace_back(Kind, *R)`.
================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:75
+}
+bool operator!=(const SemanticToken &Lhs, const SemanticToken &Rhs) {
+ return !(Lhs == Rhs);
----------------
we probably don't need the `!=` operator in this patch.
================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:13
+#include "ClangdUnit.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+
----------------
the RecursiveASTVisitor.h should be in the .cpp file.
================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:32
+
+std::vector<SemanticToken> getSemanticHighlights(ParsedAST &AST);
+
----------------
needs high-level comments.
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:24
+
+std::vector<SemanticToken> makeSemanticTokens(std::vector<Range> Ranges,
+ SemanticHighlightKind Kind) {
----------------
jvikstrom wrote:
> hokein wrote:
> > we are passing a copy here, use llvm::ArrayRef.
> I changed to pass a const vector & instead. Is that alright as well?
yeah, that works as well, llvm prefers `llvm::ArrayRef`, but up to you.
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:47
+TEST(SemanticTokenCollector, GetsCorrectTokens) {
+ const char *TestCases[] = {
+ R"cpp(
----------------
the code seems not clang-format, could you run clang-format on your patch?
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
