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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:365
                               Callback<tooling::Replacements> CB) {
-  auto Action = [Sel](decltype(CB) CB, std::string File,
-                            std::string TweakID,
-                            Expected<InputsAndAST> InpAST) {
+  auto Action = [Sel](decltype(CB) CB, std::string File, std::string TweakID,
+                      Expected<InputsAndAST> InpAST) {
----------------
Can you revert this change?


================
Comment at: clang-tools-extra/clangd/FindSymbols.h:16
 #include "Protocol.h"
+#include "index/Index.h"
 #include "llvm/ADT/StringRef.h"
----------------
this should rather be `clang/Index/IndexSymbol.h`. `index::SymbolKind` resides in that header.


================
Comment at: clang-tools-extra/clangd/FindSymbols.h:21
+class ASTContext;
+class NamedDecl;
+
----------------
I don't think these two are necessary inside header file.


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:871
+  O.map("children", I.children);
+  return true;
+}
----------------
Shouldn't we fail if some of the above fields(like `name`) fails to decode ?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:885
+static Optional<TypeHierarchyItem>
+getTypeHierarchy(const CXXRecordDecl &CXXRD, ASTContext &ASTCtx, int Levels,
+                 TypeHierarchyDirection Direction) {
----------------
nridge wrote:
> sammccall wrote:
> > 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`.
> If we remove "levels" here, should we introduce some kind of guard for infinite recursion, in case the user writes something like:
> 
> ```
> template <int N>
> struct S : S<N + 1> {};
> 
> S<0> s;
> ```
clang should be limiting recursive template instantiations. Also since we are just traversing the AST produced by clang, it should never be infinite, but still a nice test case can you add one?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:918
+
+  return CXXRD;
+}
----------------
Nit: Get rid of CXXRD and return inside if/else branches.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:924
+
+  for (auto It = CXXRD->bases_begin(); It != CXXRD->bases_end(); It++) {
+    const CXXRecordDecl *ParentDecl = nullptr;
----------------
why not use a for-range loop ? (`CXXRD->bases()`)


================
Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1562
+    const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+    EXPECT_EQ(nullptr, RD);
+  }
----------------
Can you put a TODO?


================
Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1809
+    llvm::Optional<TypeHierarchyItem> Result =
+        getTypeHierarchy(AST, Pt, 10, TypeHierarchyDirection::Parents);
+    ASSERT_TRUE(bool(Result));
----------------
I suppose parent resolving should not depend on level, so what about setting it to `0` instead of `10` so that test is consistent even after "child resolution" implementation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370