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

[Sam McCall via Phabricator via cfe-commits]

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