Page MenuHomePhabricator

(WIP) Type hierarchy
Changes PlannedPublic

Authored by simark on Aug 27 2018, 8:46 AM.

Details

Reviewers
None
Summary

This is a work in progress patch to support an eventual "get type
hierarchy" request that does not exist in the LSP today. I only plan to
support getting parent types (i.e. base classes) for now, because it
doesn't require touching the index. Finding child classes would come
later.

Event Timeline

simark created this revision.Aug 27 2018, 8:46 AM
simark planned changes to this revision.Aug 27 2018, 8:47 AM
kadircet added inline comments.Aug 27 2018, 8:22 PM
clangd/Protocol.h
889

I think comment and the name is in contradiction here, do you mean DefinesMethod?

clangd/XRefs.cpp
669

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.

693

If you plan to keep it for later debugging info, consider using {d,v}log

unittests/clangd/XRefsTests.cpp
1075

NIT: Maybe format test code as well.

simark added inline comments.Aug 28 2018, 8:02 AM
clangd/Protocol.h
889

Actually I think the comment is wrong. Even if we only see a declaration of the method, it's enough to say that this type has its own version of the method.

clangd/XRefs.cpp
669

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.

693

Yes of course :)

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

clangd/Protocol.h
891

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.

clangd/XRefs.cpp
669

See the code that actually computes a list for override_methods(): Sema::AddOverriddenMethods.

732

Why special-case the variables? Maybe just return empty results for consistency with other use-cases (typedefs, etc)?

741

Maybe also return all base types for classes?

kadircet added inline comments.Aug 28 2018, 1:02 PM
clangd/ProtocolHandlers.cpp
70

LSP doesn't have such an entry, maybe we should use workspace/executeCommand. WDYT @ilya-biryukov

clangd/XRefs.cpp
669

Did you mean to link to a particular line?

Yeah sorry, I was trying to link the function ilya mentioned.
https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp#L7615

Since you also mentioned checking non-virtual method as well you might wanna look at https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp#L7555 as well.

Also I got the chance to look the rest of the patch, as I mentioned above you are already checking two methods iff one lies in the others base classes. So you can simply check for name equality and not being an overload case.

ilya-biryukov added inline comments.
clangd/XRefs.cpp
669

Also wanted to bring up what @sammccall already mentioned on clangd-dev: we probably shouldn't show methods that hide base methods rather than override the virtual base methods (at least, by default).

Those might be useful, but unless we can have a clear UI indicator of whether a method is overriding a virtual base method or is hiding some other method, it would to add more confusion than benefit IMO.

simark added inline comments.Aug 29 2018, 8:06 AM
clangd/Protocol.h
891

In my prototype, I fetch the whole parent hierarchy in a single request. In the proposal at

https://github.com/Microsoft/vscode-languageserver-node/pull/346

it has been suggested to only fetch one level at the time, and have the client issue as many request as it wants (until possibly getting the whole hierarchy). I don't know what the protocol will end up like.

clangd/ProtocolHandlers.cpp
70

I don't intend to merge this patch before the protocol actually defines the way to get type hierarchy. So this will be updated to reflect what the protocol will actually define.

clangd/XRefs.cpp
669

Also I got the chance to look the rest of the patch, as I mentioned above you are already checking two methods iff one lies in the others base classes. So you can simply check for name equality and not being an overload case.

To check for an overload case, you would use Sema::IsOverload? Do we have access to a Sema object in this context?

Also wanted to bring up what @sammccall already mentioned on clangd-dev: we probably shouldn't show methods that hide base methods rather than override the virtual base methods (at least, by default).

Those might be useful, but unless we can have a clear UI indicator of whether a method is overriding a virtual base method or is hiding some other method, it would to add more confusion than benefit IMO.

Ok, I was basing my implementation on the behavior of Eclipse CDT, I don't think it differentiates virtual and non-virtual methods in that context. It just shows you Types declaring "method". But if you think it's better to leave them out, I'm fine with that.

732

I thought it would be useful as a shortcut, to be able to pop up the type hierarchy from a reference to a variable. But I guess to be really useful, it would need to work with pointers to. For example

Parent *parent = ...;
...
par^ent->method ();

Doing "type hierarchy" at the caret should open the type hierarchy for Parent, but it probably doesn't work in my patch. It's not an essential feature though, especially not for a first version.

741

Can you give an example? I think that's already what it does, but maybe I don't understand what you mean, or you found a bug.