simark added inline comments.
================ Comment at: clangd/Protocol.h:891 + + std::vector<TypeHierarchyResult> Parents; + ---------------- ilya-biryukov wrote: > 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. 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. ================ Comment at: clangd/ProtocolHandlers.cpp:70 Register("textDocument/hover", &ProtocolCallbacks::onHover); + Register("textDocument/typeHierarchy", &ProtocolCallbacks::onTypeHierarchy); Register("textDocument/documentSymbol", &ProtocolCallbacks::onDocumentSymbol); ---------------- kadircet wrote: > LSP doesn't have such an entry, maybe we should use [[ > https://microsoft.github.io/language-server-protocol/specification#workspace_executeCommand > | workspace/executeCommand ]]. WDYT @ilya-biryukov 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. ================ Comment at: clangd/XRefs.cpp:669 + const CXXMethodDecl *Candidate) { + // FIXME: How do I determine if Method overrides Candidate? + ---------------- ilya-biryukov wrote: > kadircet wrote: > > ilya-biryukov wrote: > > > 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. > > > 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. > 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. > 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. ================ 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) { ---------------- ilya-biryukov wrote: > Why special-case the variables? Maybe just return empty results for > consistency with other use-cases (typedefs, etc)? 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. ================ Comment at: clangd/XRefs.cpp:741 + if (CXXRD) + return getTypeHierarchy(CXXRD, Method, SourceMgr); + ---------------- ilya-biryukov wrote: > Maybe also return all base types for classes? 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. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51311 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
