[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Nov 23 13:37:22 PST 2018


sammccall added a comment.

Thanks, this looks good! Just nits, and please do port most of the test cases to unit tests.



================
Comment at: clangd/ClangdServer.cpp:529
+
+  WorkScheduler.runWithAST("CursorInfo", File, Bind(Action, std::move(CB)));
+}
----------------
nit: SymbolInfo


================
Comment at: clangd/Protocol.cpp:429
+  return json::Object{
+      {"name", P.name},
+      {"containerName", P.containerName},
----------------
for each of the attributes that can be logically absent, we should probably serialize it conditionally (or serialize it as null).

We're usually happy enough to use sentinel values like "" to mean missing internally, but we try not to send them over the wire.
(SymbolInformation isn't a great example unfortunately...)


================
Comment at: clangd/Protocol.h:691
+
+  SymbolID ID;
+};
----------------
Optional<SymbolID>?
"" is a reasonable "absent" value for strings, but we don't particularly have one for symbolID


================
Comment at: clangd/XRefs.cpp:771
+        ContainerNameRef.consume_back("::");
+      } else {
+        const auto *DC = ND->getDeclContext();
----------------
nit: just `else if (const auto* ParentND = dyn_cast_or_null<NamedDecl>(ND->getDeclContext())`?


================
Comment at: clangd/XRefs.cpp:785
+    }
+    Results.emplace_back(std::move(NewSymbol));
+  }
----------------
nit: push_back (and below)


================
Comment at: clangd/index/SymbolID.h:58
+
+llvm::hash_code hash_value(const SymbolID &ID);
+
----------------
nit: missing include of llvm/ADT/Hashing.h


================
Comment at: test/clangd/cursor-info.test:1
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
----------------
(this file should be renamed)


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

https://reviews.llvm.org/D54799





More information about the cfe-commits mailing list