ilya-biryukov added a subscriber: sammccall.
ilya-biryukov added inline comments.
================
Comment at: clangd/XRefs.cpp:669
+ const CXXMethodDecl *Candidate) {
+ // FIXME: How do I determine if Method overrides Candidate?
+
----------------
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.
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
