[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