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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a subscriber: sammccall.
kadircet added a comment.

Implementation LG, but I am not sure about adding a functionality on a proposal that might change. WDYT @sammccall ?



================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:16
 #include "SourceCode.h"
+#include "XRefs.h"
 #include "index/Index.h"
----------------
it seems like there is no change in file requiring this


================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:218
   }
+
   return SI;
----------------
can you revert this one as well ?


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:817
+bool fromJSON(const llvm::json::Value &E, TypeHierarchyDirection &Out) {
+  if (auto T = E.getAsInteger()) {
+    if (*T < static_cast<int>(TypeHierarchyDirection::Children) ||
----------------
nit: get rid of nesting by
```
auto T = E.getas..;
if(!T)
  return false;
if(*T not in rage)
  return false;
Out = cast(*T);
return true;
```


================
Comment at: clang-tools-extra/clangd/Protocol.h:1026
+  /// When not defined, it is treated as `0`.
+  llvm::Optional<int> resolve;
+
----------------
why not just use an int then ?


================
Comment at: clang-tools-extra/clangd/Protocol.h:1046
+  /// It is `false` by default.
+  llvm::Optional<bool> deprecated;
+
----------------
again we can just store a bool, and only serialize if it is true


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:876
+  Optional<TypeHierarchyItem> Result = declToTypeHierarchyItem(ASTCtx, CXXRD);
+  if (Result && Levels > 0) {
+    if (Direction == TypeHierarchyDirection::Parents ||
----------------
again reduce nesting


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:891
+        if (Optional<TypeHierarchyItem> ParentSym =
+                getTypeHierarchy(*ParentDecl, ASTCtx, Levels - 1, Direction)) {
+          Result->parents->emplace_back(std::move(*ParentSym));
----------------
A problem that might occur when you add children traversal:
do we really want to include current decl in children of parent when we have `BOTH` as direction? If so, maybe we should rather cache the results? Maybe leave a todo/fixme ?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:917
+  if (Symbols.Decls.empty())
+    return {};
+
----------------
llvm::None


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:925
+    CXXRD = VD->getType().getTypePtr()->getAsCXXRecordDecl();
+  } else if (const CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(D)) {
+    // If this is a method, use the type of the class.
----------------
what about member fields ?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:932
+
+  if (CXXRD) {
+    int ResolveLevels = Resolve.getValueOr(0);
----------------
reduce nesting


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:935
+    TypeHierarchyDirection ResolveDirection =
+        Direction.getValueOr(TypeHierarchyDirection::Parents);
+    return getTypeHierarchy(*CXXRD, ASTCtx, ResolveLevels, ResolveDirection);
----------------
Is this mentioned in proposal?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:939
+
+  return {};
+}
----------------
llvm::None


================
Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1355
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]]
+{
----------------
can you format the code?


================
Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1387
+
+  for (auto Pt : {"p1", "p2", "p3", "p4"}) {
+    llvm::Optional<TypeHierarchyItem> Result = getTypeHierarchy(
----------------
You can use `Source.points()` and use `Pt` directly instead of doing `Source.point(pt)`. Same for other tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370