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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
