[PATCH] D89296: [clangd] Call hierarchy (Protocol layer)

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:1210
+
+llvm::json::Value toJSON(const CallHierarchyItem &I) {
+  llvm::json::Object Result{
----------------
we should also populate tags and detail conditionally. rather than providing empty strings/vectors.


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:1239
+llvm::json::Value toJSON(const CallHierarchyOutgoingCall &C) {
+  return llvm::json::Object{{"from", C.To}, {"fromRanges", C.FromRanges}};
+}
----------------
s/"from"/"to"


================
Comment at: clang-tools-extra/clangd/Protocol.h:1395
+  /// The name of this item.
+  std::string Name;
+
----------------
we use exactly the same names in protocol here, even if they don't adhere to the style guide. i.e. `s/Name/name`. same for others.


================
Comment at: clang-tools-extra/clangd/Protocol.h:1411
+  /// whitespace but everything else, e.g. comments and code.
+  Range Rng;
+
----------------
s/Rng/range


================
Comment at: clang-tools-extra/clangd/Protocol.h:1420
+  /// hierarchy item in an incomingCalls or outgoingCalls request.
+  llvm::Optional<std::string> Data;
+};
----------------
i would drop the optional, as there's no difference between empty vs none.


================
Comment at: clang-tools-extra/clangd/Protocol.h:1449
+/// The parameter of a `textDocument/prepareCallHierarchy` request.
+struct CallHierarchyPrepareParams : public TextDocumentPositionParams {};
+
----------------
could you put this before `CallHierarchyItem` ?


================
Comment at: clang-tools-extra/clangd/Protocol.h:1452
+/// The parameter of a `callHierarchy/incomingCalls` request.
+struct CallHierarchyIncomingCallsParams {
+  CallHierarchyItem Item;
----------------
could you put this before `CallHierarchyIncomingCall`, same for outgoing


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89296