[PATCH] D51311: (WIP) Type hierarchy

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks for looking into this. Would be cool to get this supported after the proposal is finalized.



================
Comment at: clangd/Protocol.h:891
+
+  std::vector<TypeHierarchyResult> Parents;
+
----------------
What is the proposal to add children (i.e. overriden methods) to the hierarchy?
This seems like a place where we might want to have multiple requests from the clients when expanding the nodes.


================
Comment at: clangd/XRefs.cpp:669
+                          const CXXMethodDecl *Candidate) {
+  // FIXME: How do I determine if Method overrides Candidate?
+
----------------
simark wrote:
> kadircet wrote:
> > https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp
> > 
> > you can check this sample, which simply traverses all base classes and gets methods with the same name, then checks whether one is overload of the other. If it they are not overloads then one in the base classes gets overriden by the other.
> > 
> > 
> > Another approach could be to search for one method in others overriden_methods.
> > 
> > But they are both inefficient, I would suggest calling these functions only when one of them is defined in the base class of other then you can just check for name equality and not being an overload.
> > https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp
> 
> Did you mean to link to a particular line?
> 
> > you can check this sample, which simply traverses all base classes and gets methods with the same name, then checks whether one is overload of the other. If it they are not overloads then one in the base classes gets overriden by the other.
> 
> > Another approach could be to search for one method in others overriden_methods.
> 
> I have tried using `overriden_methods`, but it only contains methods marked virtual.  For this feature, I would like to consider non-virtual methods too.
> 
> > But they are both inefficient, I would suggest calling these functions only when one of them is defined in the base class of other then you can just check for name equality and not being an overload.
> 
> I am not sure I understand, but maybe it will be clearer when I will see the sample.
See the code that actually computes a list for `override_methods()`: Sema::AddOverriddenMethods.


================
Comment at: clangd/XRefs.cpp:732
+    // If this is a variable, use the type of the variable.
+    CXXRD = VD->getType().getTypePtr()->getAsCXXRecordDecl();
+  } else if (Method) {
----------------
Why special-case the variables? Maybe just return empty results for consistency with other use-cases (typedefs, etc)?


================
Comment at: clangd/XRefs.cpp:741
+  if (CXXRD)
+    return getTypeHierarchy(CXXRD, Method, SourceMgr);
+
----------------
Maybe also return all base types for classes?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51311