[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Hi Nathan,
Really sorry about the delay here.
I'm actually out on leave at the moment, and didn't get things wrapped up before I went.
(I'm 1 week into 4 week parental leave, and got sick the week I was supposed to be handing things off).

@kadircet, can you finish the review here so we can get this landed?
My main concern is I think this can be further simplified by always fetching all ancestors, and treating "fill descendants" as basically a separate operation.
If it's not clear how to proceed there, we can land this and I can work on simplifying when I get back.
Cheers, Sam



================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:530
+  auto Action = [Pos, Resolve,
+                 Direction](Callback<Optional<TypeHierarchyItem>> CB,
+                            Expected<InputsAndAST> InpAST) {
----------------
nit: `decltype(CB) CB` is I think a little more readable (once you're used to the idiom)


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:830
+    return false;
+  if (auto *Resolve = Params.getAsObject()->get("resolve")) {
+    if (!fromJSON(*Resolve, R.resolve)) {
----------------
use ObjectMapper here? It's more idiomatic and avoids the issues below.


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:831
+  if (auto *Resolve = Params.getAsObject()->get("resolve")) {
+    if (!fromJSON(*Resolve, R.resolve)) {
+      R.resolve = 0; // default value if not specified
----------------
this seems confused - if there is a "resolve" field, but it's not a valid integer, then we should return false (fail to decode)


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:832
+    if (!fromJSON(*Resolve, R.resolve)) {
+      R.resolve = 0; // default value if not specified
+    }
----------------
prefer to initialize it inline in the struct, rather than manually initializing in failure cases.
As written, if the "resolve" field isn't present at all, the field will be uninitialized.


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:860
+    Result["children"] = I.children;
+  // Older gcc cannot compile 'return Result', even though it is legal.
+  return llvm::json::Value(std::move(Result));
----------------
nit: this happens in lots of places, and we don't usually add a comment in each.
`return std::move(Result);` should be enough.

(technically I think this *isn't* legal in C++11, as DR1579 was only addressed in 14. But most compilers backport it...)


================
Comment at: clang-tools-extra/clangd/Protocol.h:1025
+  /// The hierarchy levels to resolve. `0` indicates no level.
+  int resolve;
+
----------------
`= 0`


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:885
+static Optional<TypeHierarchyItem>
+getTypeHierarchy(const CXXRecordDecl &CXXRD, ASTContext &ASTCtx, int Levels,
+                 TypeHierarchyDirection Direction) {
----------------
The scope and complexity of this function seems unneccesarily large:
 - it's (in future?) capable of resolving in both directions
 - it calls itself recursively, modifying TypeHierarchyDirection to control the search
 - it handles levels, even though this optimization is only important for derived types

Can we restrict this to retrieving (all) recursive base types?
i.e. `Optional<TypeHierarchyItem> getTypeAncestors(const CXXRecordDecl &CXXRD, ASTContext &Ctx)`
Then later, subtype resolution can be the job of another function: `resolveTypeDescendants(TypeHierarchyItem& Item, int depth)`

That way the recursion of getTypeAncestors is simpler to understand, as it has much smaller scope. And code sharing between the two LSP calls is clearer: fetching type hierarchy is a call to `getTypeAncestors` and a call to `resolveTypeDescendants` (if direction is children or both, and resolve is > 0), and resolving a hierarchy is a call to `resolveTypeDescendants`.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:990
+
+  TypeHierarchyDirection ResolveDirection =
+      Direction.getValueOr(TypeHierarchyDirection::Parents);
----------------
this kind of interpreting of request messages should be a few layers up: probably TypeHierarchyRequest should just have a `TypeHierarchyDirection direction = Parents` and replace it if present while parsing.


================
Comment at: clang-tools-extra/clangd/XRefs.h:65
+/// Given a record type declaration, find its base (parent) types.
+std::vector<const CXXRecordDecl *> typeAncestors(const CXXRecordDecl *CXXRD);
+
----------------
the name says "ancestors" (i.e. recursive) but the implementation is parents (non-recursive). I guess the name should change?


================
Comment at: clang-tools-extra/clangd/XRefs.h:72
+
+/// Convert a a URI (such as that returned by toURI()) into a form suitable
+/// for use in protocol replies (e.g. Location.uri, DocumentSymbol.uri).
----------------
XRefs.h isn't a suitable place to expose such a function.
It seems to be called only in one place in XRefs.cpp now, which doesn't need to be changed in this patch, can we revert the change?

If you do need to keep the function, I think it can be private.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370