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

[Haojian Wu via Phabricator via cfe-commits]

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