[PATCH] D51311: (WIP) Type hierarchy
simark added inline comments.
Comment at: clangd/Protocol.h:891
+
+ std::vector 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", ::onHover);
+ Register("textDocument/typeHierarchy", ::onTypeHierarchy);
Register("textDocument/documentSymbol",
::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`?
[PATCH] D51311: (WIP) Type hierarchy
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
[PATCH] D51311: (WIP) Type hierarchy
kadircet added inline comments.
Comment at: clangd/ProtocolHandlers.cpp:70
Register("textDocument/hover", ::onHover);
+ Register("textDocument/typeHierarchy", ::onTypeHierarchy);
Register("textDocument/documentSymbol",
::onDocumentSymbol);
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
Comment at: clangd/XRefs.cpp:669
+ const CXXMethodDecl *Candidate) {
+ // FIXME: How do I determine if Method overrides Candidate?
+
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.
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
[PATCH] D51311: (WIP) Type hierarchy
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 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51311: (WIP) Type hierarchy
simark added inline comments.
Comment at: clangd/Protocol.h:889
+ // Does this node implement the method targeted by the request?
+ bool DeclaresMethod;
+
kadircet wrote:
> I think comment and the name is in contradiction here, do you mean
> DefinesMethod?
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.
Comment at: clangd/XRefs.cpp:669
+ const CXXMethodDecl *Candidate) {
+ // FIXME: How do I determine if Method overrides Candidate?
+
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.
Comment at: clangd/XRefs.cpp:693
+
+std::cerr << " >>> A parent is " << ParentDecl->getName().str()
+ << std::endl;
kadircet wrote:
> If you plan to keep it for later debugging info, consider using {d,v}log
Yes of course :)
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
[PATCH] D51311: (WIP) Type hierarchy
kadircet added inline comments.
Comment at: clangd/Protocol.h:889
+ // Does this node implement the method targeted by the request?
+ bool DeclaresMethod;
+
I think comment and the name is in contradiction here, do you mean
DefinesMethod?
Comment at: clangd/XRefs.cpp:669
+ const CXXMethodDecl *Candidate) {
+ // FIXME: How do I determine if Method overrides Candidate?
+
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.
Comment at: clangd/XRefs.cpp:693
+
+std::cerr << " >>> A parent is " << ParentDecl->getName().str()
+ << std::endl;
If you plan to keep it for later debugging info, consider using {d,v}log
Comment at: unittests/clangd/XRefsTests.cpp:1075
+$ParentDef^struct Parent
+{
+ int a;
NIT: Maybe format test code as well.
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
[PATCH] D51311: (WIP) Type hierarchy
simark created this revision.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, ioeric,
ilya-biryukov.
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.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51311
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
clangd/XRefs.cpp
clangd/XRefs.h
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -27,6 +27,7 @@
namespace {
using testing::ElementsAre;
+using testing::Eq;
using testing::Field;
using testing::IsEmpty;
using testing::Matcher;
@@ -1068,6 +1069,166 @@
ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
}
+TEST(TypeHierarchy, SimpleInheritanceOnTypeOrVariable) {
+ Annotations Source(R"cpp(
+$ParentDef^struct Parent
+{
+ int a;
+};
+
+$Child1Def^struct Child1 : Parent
+{
+ int b;
+};
+
+struct Ch$p1^ild2 : Child1
+{
+ int c;
+};
+
+struct Child3 : Child2
+{
+ int d;
+};
+
+int main()
+{
+ Ch$p2^ild2 ch$p3^ild2;
+
+ parent.a = 1;
+ ch$p4^ild2.c = 1;
+}
+)cpp");
+
+ TestTU TU = TestTU::withCode(Source.code());
+ auto AST = TU.build();
+
+ llvm::Optional Result;
+
+ TypeHierarchy ExpectedResult{{TypeHierarchyResult{
+ "Child1",
+ Source.point("Child1Def"),
+ false,
+ {TypeHierarchyResult{"Parent", Source.point("ParentDef"), false, {}};
+
+ for (auto Pt : {"p1", "p2", "p3", "p4"}) {
+Result = getTypeHierarchy(AST, Source.point(Pt));
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(*Result, Eq(ExpectedResult));
+ }
+}
+
+TEST(TypeHierarchy, MultipleInheritanceOnTypeOrVariable) {
+ Annotations Source(R"cpp(
+$Parent1Def^struct Parent1
+{
+ int a;
+};
+
+$Parent2Def^struct Parent2
+{
+ int b;
+};
+
+$Parent3Def^struct Parent3 : Parent2
+{
+ int c;
+};
+
+struct Ch$c1^ild : Parent1, Parent3
+{
+ int d;
+};
+
+int main()
+{
+ Ch$c2^ild ch$c3^ild;
+
+ ch$c4^ild.a = 1;
+}
+)cpp");
+
+ TestTU TU = TestTU::withCode(Source.code());
+ auto AST = TU.build();
+
+ llvm::Optional Result;
+ TypeHierarchy ExpectedResult{{
+ TypeHierarchyResult{"Parent1", Source.point("Parent1Def"), false, {}},
+ TypeHierarchyResult{
+ "Parent3",
+ Source.point("Parent3Def"),
+ false,
+ {TypeHierarchyResult{
+ "Parent2", Source.point("Parent2Def"), false, {,
+ }};
+
+ for (auto Pt : {"c1", "c2", "c3", "c4"}) {
+Result = getTypeHierarchy(AST, Source.point(Pt));
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(*Result, Eq(ExpectedResult));
+ }
+}
+
+TEST(TypeHierarchy, OnMethod) {
+ Annotations Source(R"cpp(
+$ParentDef^struct Parent
+{
+ void method ();
+ void method () const;
+ void method (int x);
+ void method (char x);
+};
+
+$Child1Def^struct Child1 : Parent
+{
+ void method ();
+ void method (char x);
+};
+
+struct Child2 : Child1
+{
+ void met$p1^hod ();
+ void met$p2^hod (int x);
+};
+
+struct Child3 : Child2
+{
+ void method (int x);
+};
+)cpp");
+
+ TestTU TU = TestTU::withCode(Source.code());
+ auto AST = TU.build();
+
+ ASSERT_TRUE(AST.getDiagnostics().empty());
+
+ {
+TypeHierarchy ExpectedResult{{TypeHierarchyResult{
+"Child1",
+Source.point("Child1Def"),
+true,
+{TypeHierarchyResult{"Parent", Source.point("ParentDef"), true, {}};
+
+llvm::Optional Result =
+getTypeHierarchy(AST, Source.point("p1"));
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(*Result, Eq(ExpectedResult));
+ }
+
+ {
+TypeHierarchy ExpectedResult{{TypeHierarchyResult{
+"Child1",
+Source.point("Child1Def"),
+false,
+{TypeHierarchyResult{"Parent", Source.point("ParentDef"), true, {}};
+
+llvm::Optional Result =
+getTypeHierarchy(AST, Source.point("p2"));
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(*Result, Eq(ExpectedResult));
+ }
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/XRefs.h
===
--- clangd/XRefs.h
+++ clangd/XRefs.h
@@ -34,6 +34,9 @@
/// Get the hover information when hovering at \p Pos.
llvm::Optional getHover(ParsedAST , Position Pos);
+/// Get the type hierarchy information at \p Pos.
+llvm::Optional getTypeHierarchy(ParsedAST , Position Pos);
+
} // namespace clangd
} // namespace clang
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -17,6
