[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

I think this is ready to continue to be reviewed :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 189065.
nridge added a comment.

Rebased.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -15,6 +15,8 @@
 #include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -25,9 +27,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +45,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(&TypeHierarchyItem::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1478,6 +1493,325 @@
   }
 }
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+// TODO: FindRecordTypeAt, TemplateSpec
+
+TEST(TypeAncestors, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast(&findDecl(AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast(&findDecl(AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast(&findDecl(AST, "Child2"));
+
+  EXPECT_THAT(typeAncestors(Parent), ElementsAre());
+  EXPECT_THAT(typeAncestors(Child1), ElementsAre(Parent));
+  EXPECT_THAT(typeAncestors(Child2), ElementsAre(Child1));
+}
+
+TEST(TypeAncestors, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : Parent1, Parent3 {
+  int d;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent1 =
+  dy

[PATCH] D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes

[Nathan Ridge via Phabricator via cfe-commits]

nridge created this revision.
nridge added reviewers: sammccall, kadircet.
Herald added subscribers: cfe-commits, jdoerfert, arphaman, mgrang, jkorous, 
MaskRay, ioeric, ilya-biryukov.
Herald added a project: clang.

This is an early draft of an implementation of type hierarchy subtypes.

There is enough here to pass some basic tests, but the implementation is
incomplete (for example, the index support is in place for MemIndex but not 
Dex, and the serialization support for YAML but not RIFF).

My objective at this point is to get some high-level feedback on the addition
of "relations" to the index model and API. If this direction seems reasonable,
I will proceed and fill in the missing pieces of the implementation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58880

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/index/FileIndex.cpp
  clang-tools-extra/clangd/index/FileIndex.h
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/MemIndex.cpp
  clang-tools-extra/clangd/index/MemIndex.h
  clang-tools-extra/clangd/index/Merge.cpp
  clang-tools-extra/clangd/index/Merge.h
  clang-tools-extra/clangd/index/Serialization.cpp
  clang-tools-extra/clangd/index/Serialization.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/index/YAMLSerialization.cpp
  clang-tools-extra/clangd/index/dex/Dex.cpp
  clang-tools-extra/clangd/index/dex/Dex.h
  clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
  clang-tools-extra/unittests/clangd/DiagnosticsTests.cpp
  clang-tools-extra/unittests/clangd/FileIndexTests.cpp
  clang-tools-extra/unittests/clangd/IndexTests.cpp
  clang-tools-extra/unittests/clangd/TestTU.cpp
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -1761,6 +1761,87 @@
   EXPECT_THAT(typeAncestors(Child3), ElementsAre());
 }
 
+SymbolID findSymbolIDByName(llvm::StringRef Name, SymbolIndex *Index) {
+  SymbolID Result;
+  FuzzyFindRequest Request;
+  Request.Query = Name;
+  Request.AnyScope = true;
+  Request.Limit = 1;
+  int ResultCount = 0;
+  Index->fuzzyFind(Request, [&](const Symbol &S) {
+Result = S.ID;
+++ResultCount;
+  });
+  EXPECT_EQ(1, ResultCount);
+  return Result;
+}
+
+std::vector collectSubtypes(SymbolID Type, SymbolIndex *Index) {
+  std::vector Result;
+  llvm::errs() << "Querying subtypes of " << Type << "\n";
+  Index->relations(Type, RelationKind::Subtype,
+   [&Result](const SymbolID &ID) { Result.push_back(ID); });
+  return Result;
+}
+
+TEST(Subtypes, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto Index = TU.index();
+
+  SymbolID Parent = findSymbolIDByName("Parent", Index.get());
+  SymbolID Child1 = findSymbolIDByName("Child1", Index.get());
+  SymbolID Child2 = findSymbolIDByName("Child2", Index.get());
+
+  EXPECT_THAT(collectSubtypes(Parent, Index.get()), ElementsAre(Child1));
+  EXPECT_THAT(collectSubtypes(Child1, Index.get()), ElementsAre(Child2));
+}
+
+TEST(Subtypes, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : Parent1, Parent3 {
+  int d;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto Index = TU.index();
+
+  SymbolID Parent1 = findSymbolIDByName("Parent1", Index.get());
+  SymbolID Parent2 = findSymbolIDByName("Parent2", Index.get());
+  SymbolID Parent3 = findSymbolIDByName("Parent3", Index.get());
+  SymbolID Child = findSymbolIDByName("Child", Index.get());
+
+  EXPECT_THAT(collectSubtypes(Parent1, Index.get()), ElementsAre(Child));
+  EXPECT_THAT(collectSubtypes(Parent2, Index.get()), ElementsAre(Parent3));
+  EXPECT_THAT(collectSubtypes(Parent3, Index.get()), ElementsAre(Child));
+}
+
 // Parts of getTypeHierarchy() are tested in more detail by the
 // FindRecordTypeAt.* and TypeAncestor.* tests above. This test exercises the
 // entire operation.
Index: clang-tools-extra/unittests/clangd/TestTU.cpp
===
--- clang-tools-extra/unittests/clangd/TestTU.cpp
+++ clang-tools-extra/unittests/clangd/TestTU.cpp
@@ -65,7 +65,9 @@
 
 std::unique_ptr TestTU::index() const {
   auto AST = build();
-  auto Idx = llvm::make_unique(/*UseDex=*/true);
+  // UseDex is temporarily set to false so we can test subtypes s

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 19 inline comments as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/Protocol.cpp:830
+return false;
+  if (auto *Resolve = Params.getAsObject()->get("resolve")) {
+if (!fromJSON(*Resolve, R.resolve)) {

sammccall wrote:
> use ObjectMapper here? It's more idiomatic and avoids the issues below.
The reason I didn't use ObjectMapper is that I didn't know how to use it while 
also reusing the `fromJSON()` method for the base class, 
`TextDocumentPositionParams`.

I suppose I could just not reuse it (i.e. mention the fields of 
`TextDocumentPositionParams` in this function directly).



Comment at: clang-tools-extra/clangd/XRefs.cpp:885
+static Optional
+getTypeHierarchy(const CXXRecordDecl &CXXRD, ASTContext &ASTCtx, int Levels,
+ TypeHierarchyDirection Direction) {

sammccall wrote:
> The scope and complexity of this function seems unneccesarily large:
>  - it's (in future?) capable of resolving in both directions
>  - it calls itself recursively, modifying TypeHierarchyDirection to control 
> the search
>  - it handles levels, even though this optimization is only important for 
> derived types
> 
> Can we restrict this to retrieving (all) recursive base types?
> i.e. `Optional getTypeAncestors(const CXXRecordDecl 
> &CXXRD, ASTContext &Ctx)`
> Then later, subtype resolution can be the job of another function: 
> `resolveTypeDescendants(TypeHierarchyItem& Item, int depth)`
> 
> That way the recursion of getTypeAncestors is simpler to understand, as it 
> has much smaller scope. And code sharing between the two LSP calls is 
> clearer: fetching type hierarchy is a call to `getTypeAncestors` and a call 
> to `resolveTypeDescendants` (if direction is children or both, and resolve is 
> > 0), and resolving a hierarchy is a call to `resolveTypeDescendants`.
If we remove "levels" here, should we introduce some kind of guard for infinite 
recursion, in case the user writes something like:

```
template 
struct S : S {};

S<0> s;
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 189092.
nridge marked 2 inline comments as done.
nridge added a comment.

Address latest review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -15,6 +15,8 @@
 #include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -25,9 +27,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +45,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(&TypeHierarchyItem::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1478,6 +1493,323 @@
   }
 }
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+TEST(TypeParents, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast(&findDecl(AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast(&findDecl(AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast(&findDecl(AST, "Child2"));
+
+  EXPECT_THAT(typeParents(Parent), ElementsAre());
+  EXPECT_THAT(typeParents(Child1), ElementsAre(Parent));
+  EXPECT_THAT(typeParents(Child2), ElementsAre(Child1));
+}
+
+TEST(TypeParents, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : Parent1, Parent3 {
+  int d;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent1 =

[PATCH] D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 189093.
nridge added a comment.

Rebased onto updated D56370 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58880/new/

https://reviews.llvm.org/D58880

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/index/FileIndex.cpp
  clang-tools-extra/clangd/index/FileIndex.h
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/MemIndex.cpp
  clang-tools-extra/clangd/index/MemIndex.h
  clang-tools-extra/clangd/index/Merge.cpp
  clang-tools-extra/clangd/index/Merge.h
  clang-tools-extra/clangd/index/Serialization.cpp
  clang-tools-extra/clangd/index/Serialization.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/index/YAMLSerialization.cpp
  clang-tools-extra/clangd/index/dex/Dex.cpp
  clang-tools-extra/clangd/index/dex/Dex.h
  clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
  clang-tools-extra/unittests/clangd/DiagnosticsTests.cpp
  clang-tools-extra/unittests/clangd/FileIndexTests.cpp
  clang-tools-extra/unittests/clangd/IndexTests.cpp
  clang-tools-extra/unittests/clangd/TestTU.cpp
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -1759,6 +1759,87 @@
   EXPECT_THAT(typeParents(Child3), ElementsAre());
 }
 
+SymbolID findSymbolIDByName(llvm::StringRef Name, SymbolIndex *Index) {
+  SymbolID Result;
+  FuzzyFindRequest Request;
+  Request.Query = Name;
+  Request.AnyScope = true;
+  Request.Limit = 1;
+  int ResultCount = 0;
+  Index->fuzzyFind(Request, [&](const Symbol &S) {
+Result = S.ID;
+++ResultCount;
+  });
+  EXPECT_EQ(1, ResultCount);
+  return Result;
+}
+
+std::vector collectSubtypes(SymbolID Type, SymbolIndex *Index) {
+  std::vector Result;
+  llvm::errs() << "Querying subtypes of " << Type << "\n";
+  Index->relations(Type, RelationKind::Subtype,
+   [&Result](const SymbolID &ID) { Result.push_back(ID); });
+  return Result;
+}
+
+TEST(Subtypes, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto Index = TU.index();
+
+  SymbolID Parent = findSymbolIDByName("Parent", Index.get());
+  SymbolID Child1 = findSymbolIDByName("Child1", Index.get());
+  SymbolID Child2 = findSymbolIDByName("Child2", Index.get());
+
+  EXPECT_THAT(collectSubtypes(Parent, Index.get()), ElementsAre(Child1));
+  EXPECT_THAT(collectSubtypes(Child1, Index.get()), ElementsAre(Child2));
+}
+
+TEST(Subtypes, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : Parent1, Parent3 {
+  int d;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto Index = TU.index();
+
+  SymbolID Parent1 = findSymbolIDByName("Parent1", Index.get());
+  SymbolID Parent2 = findSymbolIDByName("Parent2", Index.get());
+  SymbolID Parent3 = findSymbolIDByName("Parent3", Index.get());
+  SymbolID Child = findSymbolIDByName("Child", Index.get());
+
+  EXPECT_THAT(collectSubtypes(Parent1, Index.get()), ElementsAre(Child));
+  EXPECT_THAT(collectSubtypes(Parent2, Index.get()), ElementsAre(Parent3));
+  EXPECT_THAT(collectSubtypes(Parent3, Index.get()), ElementsAre(Child));
+}
+
 // Parts of getTypeHierarchy() are tested in more detail by the
 // FindRecordTypeAt.* and TypeParents.* tests above. This test exercises the
 // entire operation.
Index: clang-tools-extra/unittests/clangd/TestTU.cpp
===
--- clang-tools-extra/unittests/clangd/TestTU.cpp
+++ clang-tools-extra/unittests/clangd/TestTU.cpp
@@ -65,7 +65,9 @@
 
 std::unique_ptr TestTU::index() const {
   auto AST = build();
-  auto Idx = llvm::make_unique(/*UseDex=*/true);
+  // UseDex is temporarily set to false so we can test subtypes support
+  // before implementing it in Dex.
+  auto Idx = llvm::make_unique(/*UseDex=*/false);
   Idx->updatePreamble(Filename, AST.getASTContext(), AST.getPreprocessorPtr(),
   AST.getCanonicalIncludes());
   Idx->updateMain(Filename, AST);
Index: clang-tools-extra/unittests/clangd/IndexTests.cpp
===
--- clang-tools-extra/unittests/clangd/IndexTests.cpp
+++ clang-tools-extra/unittests/clangd/IndexTests.cpp
@@ -77,8 +77,9 @@
   auto Token = st

[PATCH] D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes

[Nathan Ridge via Phabricator via cfe-commits]

nridge removed a reviewer: sammccall.
nridge added a comment.

Removing Sam as reviewer as he's on parental leave.

Kadir, if you have other reviewers to suggest to provide feedback on the API 
changes here, please feel free to add them. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58880/new/

https://reviews.llvm.org/D58880



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 189114.
nridge added a comment.

Address a couple of outstanding TODOs


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -15,6 +15,8 @@
 #include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -25,9 +27,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +45,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(&TypeHierarchyItem::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1478,6 +1493,337 @@
   }
 }
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+TEST(TypeParents, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast(&findDecl(AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast(&findDecl(AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast(&findDecl(AST, "Child2"));
+
+  EXPECT_THAT(typeParents(Parent), ElementsAre());
+  EXPECT_THAT(typeParents(Child1), ElementsAre(Parent));
+  EXPECT_THAT(typeParents(Child2), ElementsAre(Child1));
+}
+
+TEST(TypeParents, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : Parent1, Parent3 {
+  int d;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent1 =
+  dyn_cast(&findDecl(AST, "P

[PATCH] D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 189275.
nridge added a comment.

Add tests involving templates and template specializations


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58880/new/

https://reviews.llvm.org/D58880

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/index/FileIndex.cpp
  clang-tools-extra/clangd/index/FileIndex.h
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/MemIndex.cpp
  clang-tools-extra/clangd/index/MemIndex.h
  clang-tools-extra/clangd/index/Merge.cpp
  clang-tools-extra/clangd/index/Merge.h
  clang-tools-extra/clangd/index/Serialization.cpp
  clang-tools-extra/clangd/index/Serialization.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/index/YAMLSerialization.cpp
  clang-tools-extra/clangd/index/dex/Dex.cpp
  clang-tools-extra/clangd/index/dex/Dex.h
  clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
  clang-tools-extra/unittests/clangd/DiagnosticsTests.cpp
  clang-tools-extra/unittests/clangd/FileIndexTests.cpp
  clang-tools-extra/unittests/clangd/IndexTests.cpp
  clang-tools-extra/unittests/clangd/TestTU.cpp
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -1773,6 +1773,175 @@
   EXPECT_THAT(typeParents(Child3), ElementsAre());
 }
 
+SymbolID findSymbolIDByName(llvm::StringRef Name, SymbolIndex *Index) {
+  SymbolID Result;
+  FuzzyFindRequest Request;
+  Request.Query = Name;
+  Request.AnyScope = true;
+  Request.Limit = 1;
+  int ResultCount = 0;
+  Index->fuzzyFind(Request, [&](const Symbol &S) {
+Result = S.ID;
+++ResultCount;
+  });
+  EXPECT_EQ(1, ResultCount);
+  return Result;
+}
+
+std::vector collectSubtypes(SymbolID Type, SymbolIndex *Index) {
+  std::vector Result;
+  Index->relations(Type, RelationKind::Subtype,
+   [&Result](const SymbolID &ID) { Result.push_back(ID); });
+  return Result;
+}
+
+TEST(Subtypes, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto Index = TU.index();
+
+  SymbolID Parent = findSymbolIDByName("Parent", Index.get());
+  SymbolID Child1 = findSymbolIDByName("Child1", Index.get());
+  SymbolID Child2 = findSymbolIDByName("Child2", Index.get());
+
+  EXPECT_THAT(collectSubtypes(Parent, Index.get()), ElementsAre(Child1));
+  EXPECT_THAT(collectSubtypes(Child1, Index.get()), ElementsAre(Child2));
+}
+
+TEST(Subtypes, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : Parent1, Parent3 {
+  int d;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto Index = TU.index();
+
+  SymbolID Parent1 = findSymbolIDByName("Parent1", Index.get());
+  SymbolID Parent2 = findSymbolIDByName("Parent2", Index.get());
+  SymbolID Parent3 = findSymbolIDByName("Parent3", Index.get());
+  SymbolID Child = findSymbolIDByName("Child", Index.get());
+
+  EXPECT_THAT(collectSubtypes(Parent1, Index.get()), ElementsAre(Child));
+  EXPECT_THAT(collectSubtypes(Parent2, Index.get()), ElementsAre(Parent3));
+  EXPECT_THAT(collectSubtypes(Parent3, Index.get()), ElementsAre(Child));
+}
+
+TEST(Subtypes, ClassTemplate) {
+  Annotations Source(R"cpp(
+struct Parent {};
+
+template 
+struct Child : Parent {};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto Index = TU.index();
+
+  SymbolID Parent = findSymbolIDByName("Parent", Index.get());
+  SymbolID Child = findSymbolIDByName("Child", Index.get());
+
+  EXPECT_THAT(collectSubtypes(Parent, Index.get()), ElementsAre(Child));
+}
+
+// This test fails because explicit class template specializations
+// are not currently indexed.
+TEST(DISABLED_Subtypes, TemplateSpec1) {
+  Annotations Source(R"cpp(
+template 
+struct Parent {};
+
+template <>
+struct Parent {};
+
+struct Child1 : Parent {};
+
+struct Child2 : Parent {};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto Index = TU.index();
+
+  SymbolID Parent = findSymbolIDByName("Parent", Index.get());
+  // Even if we start storing explicit specializations in the index,
+  // we will likely need additional changes for
+  // findSymbolIDByName("Parent") to find it.
+  SymbolID ParentSpec = findSymbolIDByName("Parent", Index.get());
+  SymbolID Child1 = findSymbolIDByName("Child1", Index.get());
+  SymbolID Child

[PATCH] D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D58880#1416754 , @kadircet wrote:

> Sorry I didn't notice the mailing thread beforehand, looks like Sam had a 
> good point regarding performing operations on types rather than 
> symbols(http://lists.llvm.org/pipermail/clangd-dev/2019-January/000241.html).
>  Does current implementation take this into account by either pointing at 
> template specializations and/or base template declarations?


I thought about this, and I believe that having the relationship be between 
symbols is sufficient.

The main utility in a type hierarchy view is to allow the user to navigate to 
base/derived classes. If a base or derived class is an //implicit// 
specialization, the desired navigation is to the corresponding template 
definition; if it's an //explicit// specialization, the desired navigation is 
to the explicit specialization's definition.

A class template will obviously have its own symbol, so that part's fine. 
Explicit specializations are not currently stored in the index, but I think it 
would make sense for them to be, for other reasons as well (e.g. a user may 
want to navigate to an explicit specialization definition via 
`"workspace/symbol"`), in which case they too will get their own symbols. So, 
in either case, the desired type hierarchy item can be obtained from a symbol.

The updated patch has tests covering these scenarios, specifically 
`Subtypes.ClassTemplate` and `Subtypes.DependentBase`. Two additional tests, 
`Subtypes.TemplateSpec1` and `Subtypes.TemplateSpec2` are disabled until we 
start storing explicit specializations in the index.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58880/new/

https://reviews.llvm.org/D58880



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D58880#1418195 , @hokein wrote:

> I think we can extend the existing Ref to support it, so that most of the 
> stuff could be reused, rather than implementing a new slab:
>
> - introduce a new RefKind, like BaseOf
> - add a new field SymbolID in Ref


I had considered this approach as well, but figured that it would be wasteful 
in terms of storage space.

My understanding is that the storage space taken up for Refs is currently 8 
bytes per Ref (4 each for the start and end positions), plus filename strings 
which are deduplicated across all refs. If we add a SymbolID, that adds an 
additional 8 bytes to each Ref. Given that Refs are numerous, and most of them 
won't use the SymbolID, that seems wasteful.

That said, I do appreciate that this is a simpler approach in terms of 
complexity, so if folks feel the right tradeoff is to take the Refs approach, I 
am open to doing that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58880/new/

https://reviews.llvm.org/D58880



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 189444.
nridge marked 10 inline comments as done.
nridge added a comment.

Address most of the latest review comments.

The infinite recursion issue remains to be fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -15,6 +15,8 @@
 #include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -25,9 +27,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +45,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(&TypeHierarchyItem::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1478,6 +1493,361 @@
   }
 }
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+// A field does not unambiguously specify a record type
+// (possible associated reocrd types could be the field's type,
+// or the type of the record that the field is a member of).
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+TEST(TypeParents, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast(&findDecl(AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast(&findDecl(AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast(&findDecl(AST, "Child2"));
+
+  EXPECT_THAT(typeParents(Parent), ElementsAre());
+  EXPECT_THAT(typeParents(Child1), ElementsAre(Parent));
+  EXPECT_THAT(typeParents(Child2), ElementsAre(Child1));
+}
+
+TEST(TypeParents, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:365
   Callback CB) {
-  auto Action = [Sel](decltype(CB) CB, std::string File,
-std::string TweakID,
-Expected InpAST) {
+  auto Action = [Sel](decltype(CB) CB, std::string File, std::string TweakID,
+  Expected InpAST) {

kadircet wrote:
> Can you revert this change?
I tried, but clang-format automatically makes the change again.

Is there some particular clang-format configuration I should apply, so it 
produces the same results? I don't currently have any configuration, i.e. I 
think it's just reading the .clang-format file in the monorepo root.



Comment at: clang-tools-extra/clangd/XRefs.cpp:885
+static Optional
+getTypeHierarchy(const CXXRecordDecl &CXXRD, ASTContext &ASTCtx, int Levels,
+ TypeHierarchyDirection Direction) {

sammccall wrote:
> kadircet wrote:
> > nridge wrote:
> > > sammccall wrote:
> > > > The scope and complexity of this function seems unneccesarily large:
> > > >  - it's (in future?) capable of resolving in both directions
> > > >  - it calls itself recursively, modifying TypeHierarchyDirection to 
> > > > control the search
> > > >  - it handles levels, even though this optimization is only important 
> > > > for derived types
> > > > 
> > > > Can we restrict this to retrieving (all) recursive base types?
> > > > i.e. `Optional getTypeAncestors(const CXXRecordDecl 
> > > > &CXXRD, ASTContext &Ctx)`
> > > > Then later, subtype resolution can be the job of another function: 
> > > > `resolveTypeDescendants(TypeHierarchyItem& Item, int depth)`
> > > > 
> > > > That way the recursion of getTypeAncestors is simpler to understand, as 
> > > > it has much smaller scope. And code sharing between the two LSP calls 
> > > > is clearer: fetching type hierarchy is a call to `getTypeAncestors` and 
> > > > a call to `resolveTypeDescendants` (if direction is children or both, 
> > > > and resolve is > 0), and resolving a hierarchy is a call to 
> > > > `resolveTypeDescendants`.
> > > If we remove "levels" here, should we introduce some kind of guard for 
> > > infinite recursion, in case the user writes something like:
> > > 
> > > ```
> > > template 
> > > struct S : S {};
> > > 
> > > S<0> s;
> > > ```
> > clang should be limiting recursive template instantiations. Also since we 
> > are just traversing the AST produced by clang, it should never be infinite, 
> > but still a nice test case can you add one?
> I think there is a point here...
> 
> Consider our `S` template with the direction reversed and a base case 
> specialized:
> ```
> template 
> struct S : S {};
> template<>
> struct S<0>{};
> 
> S<2> instance;
> ```
> 
> Now the hierarchy of `S<2>` is well defined and finite: `S<2> : S<1> : S<0>`.
> However IIUC the `CXXRecordDecl` for `S<2>` is the instantiation of the 
> primary template, whose base is `S`, which is dependent[1], so the 
> base's `CXXRecordDecl` is the primary template, whose base is `S`... 
> and we never reach the base case.
> 
> Actually I'm not sure whether this happens if the base is dependent merely 
> where it's spelled, or still dependent after instantiation? Even in the 
> latter case one can construct examples where we'll infloop:
> ```template 
> struct Foo {
>   S instance;
> }```
> Trying to get the hierarchy on `S` could infloop. (I agree these should 
> both be test cases)
> 
> What's the Right Thing?
>  - not sure about a recursion limit, as showing 10 `S`s in the hierarchy 
> is silly.
>  - not sure about bailing out on dependent types either, as knowing that e.g. 
> my `SmallSet` inherits from `SmallMap` is meaningful or useful.
>  - maybe we should bail out once we see instantiations of the same template 
> decl twice in a parent chain. I.e. keep the seen non-null 
> `CXXRecordDecl->getTemplateInstantiationPattern()`s in a set and stop 
> recursing if insertion fails.
> 
> However for this patch I'd suggest just bailing out on dependent types with a 
> TODO as the simplest, this is an edge case that can be fixed later.
> 
The current patch does actually recurse infinitely on this test case, likely 
for the reasons Sam outlined (our handling of dependent types), and eventually 
runs into the segfault.

I will update the patch to address this.



Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1562
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(nullptr, RD);
+  }

kadircet wrote:
> Can you put a TODO?
I added a comment to clarify that a field does not unambiguously specify a 
record type. i don't think there's anything further "to do" here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked an inline comment as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/XRefs.cpp:885
+static Optional
+getTypeHierarchy(const CXXRecordDecl &CXXRD, ASTContext &ASTCtx, int Levels,
+ TypeHierarchyDirection Direction) {

nridge wrote:
> sammccall wrote:
> > kadircet wrote:
> > > nridge wrote:
> > > > sammccall wrote:
> > > > > The scope and complexity of this function seems unneccesarily large:
> > > > >  - it's (in future?) capable of resolving in both directions
> > > > >  - it calls itself recursively, modifying TypeHierarchyDirection to 
> > > > > control the search
> > > > >  - it handles levels, even though this optimization is only important 
> > > > > for derived types
> > > > > 
> > > > > Can we restrict this to retrieving (all) recursive base types?
> > > > > i.e. `Optional getTypeAncestors(const 
> > > > > CXXRecordDecl &CXXRD, ASTContext &Ctx)`
> > > > > Then later, subtype resolution can be the job of another function: 
> > > > > `resolveTypeDescendants(TypeHierarchyItem& Item, int depth)`
> > > > > 
> > > > > That way the recursion of getTypeAncestors is simpler to understand, 
> > > > > as it has much smaller scope. And code sharing between the two LSP 
> > > > > calls is clearer: fetching type hierarchy is a call to 
> > > > > `getTypeAncestors` and a call to `resolveTypeDescendants` (if 
> > > > > direction is children or both, and resolve is > 0), and resolving a 
> > > > > hierarchy is a call to `resolveTypeDescendants`.
> > > > If we remove "levels" here, should we introduce some kind of guard for 
> > > > infinite recursion, in case the user writes something like:
> > > > 
> > > > ```
> > > > template 
> > > > struct S : S {};
> > > > 
> > > > S<0> s;
> > > > ```
> > > clang should be limiting recursive template instantiations. Also since we 
> > > are just traversing the AST produced by clang, it should never be 
> > > infinite, but still a nice test case can you add one?
> > I think there is a point here...
> > 
> > Consider our `S` template with the direction reversed and a base case 
> > specialized:
> > ```
> > template 
> > struct S : S {};
> > template<>
> > struct S<0>{};
> > 
> > S<2> instance;
> > ```
> > 
> > Now the hierarchy of `S<2>` is well defined and finite: `S<2> : S<1> : 
> > S<0>`.
> > However IIUC the `CXXRecordDecl` for `S<2>` is the instantiation of the 
> > primary template, whose base is `S`, which is dependent[1], so the 
> > base's `CXXRecordDecl` is the primary template, whose base is `S`... 
> > and we never reach the base case.
> > 
> > Actually I'm not sure whether this happens if the base is dependent merely 
> > where it's spelled, or still dependent after instantiation? Even in the 
> > latter case one can construct examples where we'll infloop:
> > ```template 
> > struct Foo {
> >   S instance;
> > }```
> > Trying to get the hierarchy on `S` could infloop. (I agree these should 
> > both be test cases)
> > 
> > What's the Right Thing?
> >  - not sure about a recursion limit, as showing 10 `S`s in the 
> > hierarchy is silly.
> >  - not sure about bailing out on dependent types either, as knowing that 
> > e.g. my `SmallSet` inherits from `SmallMap` is meaningful or 
> > useful.
> >  - maybe we should bail out once we see instantiations of the same template 
> > decl twice in a parent chain. I.e. keep the seen non-null 
> > `CXXRecordDecl->getTemplateInstantiationPattern()`s in a set and stop 
> > recursing if insertion fails.
> > 
> > However for this patch I'd suggest just bailing out on dependent types with 
> > a TODO as the simplest, this is an edge case that can be fixed later.
> > 
> The current patch does actually recurse infinitely on this test case, likely 
> for the reasons Sam outlined (our handling of dependent types), and 
> eventually runs into the segfault.
> 
> I will update the patch to address this.
I meant to say "eventually runs into a stack overflow".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes

[Nathan Ridge via Phabricator via cfe-commits]

nridge planned changes to this revision.
nridge added a comment.

Thanks for the detailed analysis!

For completeness, I think another advantage of option 3 is that it allows us to 
make the key for relation lookups be a (SymbolID, RelationKind) pair rather 
than just a SymbolID.

It sounds like I should continue implementing the current approach. Marking the 
revision as "changes planned" accordingly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58880/new/

https://reviews.llvm.org/D58880



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 190040.
nridge added a comment.

Address the infinite recursion issue


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -15,6 +15,8 @@
 #include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -25,9 +27,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +45,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(&TypeHierarchyItem::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1478,6 +1493,406 @@
   }
 }
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+// A field does not unambiguously specify a record type
+// (possible associated reocrd types could be the field's type,
+// or the type of the record that the field is a member of).
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+TEST(TypeParents, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast(&findDecl(AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast(&findDecl(AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast(&findDecl(AST, "Child2"));
+
+  EXPECT_THAT(typeParents(Parent), ElementsAre());
+  EXPECT_THAT(typeParents(Child1), ElementsAre(Parent));
+  EXPECT_THAT(typeParents(Child2), ElementsAre(Child1));
+}
+
+TEST(TypeParents, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : Parent1, Parent3 {
+  int d;
+};
+)cpp")

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked an inline comment as done.
nridge added a comment.

The updated patch addresses the infinite recursion issue by bailing on 
dependent bases for now, as Sam suggested. I will implement the more 
comprehensive suggested fix ("bail out once we see instantiations of the same 
template decl twice in a parent chain") in a follow-up patch. I did add all 
three testcases that have come up during discussion.

I believe all review comments to date are addressed now.




Comment at: clang-tools-extra/clangd/ClangdServer.cpp:365
   Callback CB) {
-  auto Action = [Sel](decltype(CB) CB, std::string File,
-std::string TweakID,
-Expected InpAST) {
+  auto Action = [Sel](decltype(CB) CB, std::string File, std::string TweakID,
+  Expected InpAST) {

kadircet wrote:
> nridge wrote:
> > kadircet wrote:
> > > Can you revert this change?
> > I tried, but clang-format automatically makes the change again.
> > 
> > Is there some particular clang-format configuration I should apply, so it 
> > produces the same results? I don't currently have any configuration, i.e. I 
> > think it's just reading the .clang-format file in the monorepo root.
> That's what we use as well(and your formatted version is correct), but 
> unfortunately sometimes people send out changes with wrong formatting and 
> they are hard to notice during review. Generally we try not to touch those 
> lines in irrelevant patches to keep blame clean.
> 
> If you are running clang-format directly you can instead try 
> clang-format-diff to format only changed lines. But not that crucial.
I'm not running clang-format directly, VSCode's clang-format extension is doing 
so automatically upon every file-save. I have not found an option to configure 
it to format only changed lines.

Would it help if I moved the formatting corrections to their own patch (and 
made this patch depend on that)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 190041.
nridge added a comment.

Fix a (somewhat amusing) typo where I wrote '0' instead of 'O' in a fromJSON() 
implementation

(Does the fact that this didn't cause any test failures suggest that the 
fromJSON() functions aren't exercised by any tests?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -15,6 +15,8 @@
 #include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -25,9 +27,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +45,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(&TypeHierarchyItem::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1478,6 +1493,406 @@
   }
 }
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+// A field does not unambiguously specify a record type
+// (possible associated reocrd types could be the field's type,
+// or the type of the record that the field is a member of).
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+TEST(TypeParents, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast(&findDecl(AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast(&findDecl(AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast(&findDecl(AST, "Child2"));
+
+  EXPECT_THAT(typeParents(Parent), ElementsAre());
+  EXPECT_THAT(typeParents(Child1), ElementsAre(Parent));
+  EXPECT_THAT(typeParents(Child2), ElementsAre(Child1));
+}
+
+TEST(TypeParents, MultipleInheritance) {
+  Annotati

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Unfortunately, there is a further problem: the Theia client-side implementation 
of type hierarchy has recently merged, and their code has changed so that they 
do require `typeHierarchy/resolve` to be supported. They ask for 1 level in the 
initial request, ignore any extra levels the server might send, and ask for 
extra levels using `typeHierarchy/resolve` instead.

What should we do here -- seek to change Theia, or implement 
`typeHierachy/resolve` after all?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59083: [clangd] Store explicit template specializations in index for code navigation purposes

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Is any representation of the template arguments stored in the index?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59083/new/

https://reviews.llvm.org/D59083



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59083: [clangd] Store explicit template specializations in index for code navigation purposes

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

One of the use cases I imagined for this (unrelated to D58880 
) is that if the user searches for `vector` 
(using `workspace/symbols`), they get a separate search result for the `vector` 
primary template, and a separate one for `vector`. For this to be useful, 
the server needs to print the `` as part of the search result.

(For D58880 , I would find it useful for be 
able to look up an explicit spec. by template-id in the tests, but that can 
probably be worked around.)


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59083/new/

https://reviews.llvm.org/D59083



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

> In D56370#1424180 , @nridge wrote:
> 
>> Unfortunately, there is a further problem: the Theia client-side 
>> implementation of type hierarchy has recently merged, and their code has 
>> changed so that they do require `typeHierarchy/resolve` to be supported. 
>> They ask for 1 level in the initial request, ignore any extra levels the 
>> server might send, and ask for extra levels using `typeHierarchy/resolve` 
>> instead.
>>
>> What should we do here -- seek to change Theia, or implement 
>> `typeHierachy/resolve` for supertypes after all?
> 
> 
> Looking at 
> https://github.com/theia-ide/theia/pull/3802#issuecomment-455992523 inside 
> theia's implementation of this feature, I believe these are all subject to 
> change.

Note, that is a fairly old comment, and as mentioned the PR in question has 
recently merged.

> So let's leave it as it is, even if it would mean you will be able to get 
> only one level of parents knowledge in theia for now. I believe it should be 
> addressed in theia's implementation and proposal itself(which is acutally 
> addressed by sam, 
> https://github.com/Microsoft/vscode-languageserver-node/pull/426/files#r255416980)
>  And if the conclusion happens to be in favor of theia's current 
> implementation we can always change current implementation to respond to 
> those queries as well.

Ok, I filed a Theia issue  
about it for now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D56370#1426234 , @kadircet wrote:

> Unfortunately we usually test LSP layer with lit tests, and since we don't 
> have any lit tests for this use-case it wasn't caught. Could you add a lit 
> test for overall use case? You can see examples inside 
> clang-tools-extra/test/clangd/ folder.


Are these tests written manually, or with the help or some kind of tool?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 190375.
nridge added a comment.

Address remaining review comments

(I figure out how to write a lit test)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/test/clangd/type-hierarchy.test
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp

Index: clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
@@ -0,0 +1,462 @@
+//===-- TypeHierarchyTests.cpp  ---*- C++ -*---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "Annotations.h"
+#include "ClangdUnit.h"
+#include "Compiler.h"
+#include "Matchers.h"
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "TestTU.h"
+#include "XRefs.h"
+#include "index/FileIndex.h"
+#include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/Index/IndexingAction.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::AllOf;
+using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
+using testing::IsEmpty;
+using testing::Matcher;
+using testing::Pointee;
+using testing::UnorderedElementsAreArray;
+
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(&TypeHierarchyItem::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+// A field does not unambiguously specify a record type
+// (possible associated reocrd types could be the field's type,
+// or the type of the record that the field is a member of).
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+TEST(TypeParents, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast(&findDecl(AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast(&findDecl(AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast(&findDecl(AST, "Child2"));
+
+  EXPECT_THAT(typeParents(Parent), ElementsAre()

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 7 inline comments as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/XRefs.h:62
+/// Find the record type references at \p Pos.
+const CXXRecordDecl *findRecordTypeAt(ParsedAST &AST, Position Pos);
+

ilya-biryukov wrote:
> This method looks like an implementation detail and does not align with other 
> methods in `XRefs.h` which are high-level methods that implement LSP 
> functionality.
> 
> It would be more appropriate to move it to `AST.h` or directly into the 
> `XRefs.cpp`. WDYT?
@sammccall asked for this method to be exposed in the header and some of the 
tests written in terms of it.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:154
+// the SourceManager.
+std::string toURI(const SourceManager &SM, llvm::StringRef Path,
+  llvm::StringRef FallbackDir);

ioeric wrote:
> why are we pulling this into the header? This still seems to be only used in 
> SymbolCollector.cpp.
You're right. This is a leftover from an earlier version of the patch.



Comment at: clang-tools-extra/test/clangd/type-hierarchy.test:1
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}

kadircet wrote:
> why strict-whitespace?
That's what the other lit tests seem to do, I just copied it :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 190755.
nridge marked an inline comment as done.
nridge added a comment.

Address latest review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/test/clangd/type-hierarchy.test
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp

Index: clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
@@ -0,0 +1,462 @@
+//===-- TypeHierarchyTests.cpp  ---*- C++ -*---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "Annotations.h"
+#include "ClangdUnit.h"
+#include "Compiler.h"
+#include "Matchers.h"
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "TestTU.h"
+#include "XRefs.h"
+#include "index/FileIndex.h"
+#include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/Index/IndexingAction.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::AllOf;
+using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
+using testing::IsEmpty;
+using testing::Matcher;
+using testing::Pointee;
+using testing::UnorderedElementsAreArray;
+
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(&TypeHierarchyItem::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+// A field does not unambiguously specify a record type
+// (possible associated reocrd types could be the field's type,
+// or the type of the record that the field is a member of).
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+TEST(TypeParents, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast(&findDecl(AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast(&findDecl(AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast(&findDecl(AST, "Child2"));
+
+  EXPECT_THAT(typeParents(Parent), ElementsAre());

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge created this revision.
nridge added a reviewer: kadircet.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ioeric, 
ilya-biryukov.
Herald added a project: clang.

RelationSlab is a new index data structure, similar to SymbolSlab and
RefSlab. RelationSlab stores relations between Symbols.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59407

Files:
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h

Index: clang-tools-extra/clangd/index/Index.h
===
--- clang-tools-extra/clangd/index/Index.h
+++ clang-tools-extra/clangd/index/Index.h
@@ -22,6 +22,71 @@
 namespace clang {
 namespace clangd {
 
+enum class RelationKind { Subtype };
+
+struct RelationKey {
+  SymbolID Symbol;
+  RelationKind Kind;
+
+  bool operator==(const RelationKey &Other) const {
+return Symbol == Other.Symbol && Kind == Other.Kind;
+  }
+
+private:
+  friend llvm::hash_code hash_value(const RelationKey &Key) {
+return llvm::hash_combine(Key.Symbol, static_cast(Key.Kind));
+  }
+};
+
+class RelationSlab {
+public:
+  using value_type = std::pair>;
+  using const_iterator = std::vector::const_iterator;
+  using iterator = const_iterator;
+
+  RelationSlab() = default;
+  RelationSlab(RelationSlab &&Slab) = default;
+  RelationSlab &operator=(RelationSlab &&RHS) = default;
+
+  const_iterator begin() const { return Relations.begin(); }
+  const_iterator end() const { return Relations.end(); }
+  size_t size() const { return Relations.size(); }
+  size_t numRelations() const { return NumRelations; }
+  bool empty() const { return Relations.empty(); }
+
+  size_t bytes() const {
+return sizeof(*this) + Arena.getTotalMemory() +
+   sizeof(value_type) * Relations.size();
+  }
+
+  // RelationSlab::Builder is a mutable container that can 'freeze' to
+  // RelationSlab.
+  class Builder {
+  public:
+Builder() {}
+// Adds a relation to the slab. Deep copy: Strings will be owned by the
+// slab.
+void insert(const RelationKey &Key, const SymbolID &S);
+// Consumes the builder to finalize the slab.
+RelationSlab build() &&;
+
+  private:
+llvm::BumpPtrAllocator Arena;
+llvm::DenseMap> Relations;
+  };
+
+private:
+  RelationSlab(std::vector Relations, llvm::BumpPtrAllocator Arena,
+   size_t NumRelations)
+  : Arena(std::move(Arena)), Relations(std::move(Relations)),
+NumRelations(NumRelations) {}
+
+  llvm::BumpPtrAllocator Arena;
+  std::vector Relations;
+  // Number of all relations.
+  size_t NumRelations = 0;
+};
+
 struct FuzzyFindRequest {
   /// \brief A query string for the fuzzy find. This is matched against symbols'
   /// un-qualified identifiers and should not contain qualifiers like "::".
@@ -136,4 +201,30 @@
 } // namespace clangd
 } // namespace clang
 
+namespace llvm {
+
+// Support RelationKeys as DenseMap keys.
+template <> struct DenseMapInfo {
+  static inline clang::clangd::RelationKey getEmptyKey() {
+return {DenseMapInfo::getEmptyKey(),
+clang::clangd::RelationKind::Subtype};
+  }
+
+  static inline clang::clangd::RelationKey getTombstoneKey() {
+return {DenseMapInfo::getTombstoneKey(),
+clang::clangd::RelationKind::Subtype};
+  }
+
+  static unsigned getHashValue(const clang::clangd::RelationKey &Key) {
+return hash_value(Key);
+  }
+
+  static bool isEqual(const clang::clangd::RelationKey &LHS,
+  const clang::clangd::RelationKey &RHS) {
+return LHS == RHS;
+  }
+};
+
+} // namespace llvm
+
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_INDEX_H
Index: clang-tools-extra/clangd/index/Index.cpp
===
--- clang-tools-extra/clangd/index/Index.cpp
+++ clang-tools-extra/clangd/index/Index.cpp
@@ -17,6 +17,28 @@
 namespace clang {
 namespace clangd {
 
+void RelationSlab::Builder::insert(const RelationKey &Key, const SymbolID &S) {
+  Relations[Key].push_back(S);
+}
+
+RelationSlab RelationSlab::Builder::build() && {
+  // Reallocate relations on the arena to reduce waste and indirections when
+  // reading.
+  std::vector>> Result;
+  Result.reserve(Relations.size());
+  size_t NumRelations = 0;
+  for (auto &Entry : Relations) {
+auto &Rels = Entry.second;
+
+NumRelations += Rels.size();
+auto *Array = Arena.Allocate(Rels.size());
+std::uninitialized_copy(Rels.begin(), Rels.end(), Array);
+Result.emplace_back(Entry.first,
+llvm::ArrayRef(Array, Rels.size()));
+  }
+  return RelationSlab(std::move(Result), std::move(Arena), NumRelations);
+}
+
 void SwapIndex::reset(std::unique_ptr Index) {
   // Keep the old index alive, so we don't destroy it under lock (may be slow).
   std::shared_ptr Pin;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D58880#1422494 , @kadircet wrote:

> Hi Nathan,
>
> I would also suggest splitting up current changes so that we can start 
> reviewing them, which might result in other changes in your planned changes 
> and help reduce duplicate work both on our and your side.
>  [...]
>
> As for splitting changes I would suggest something like:
>
> - RelationSlab
> - Serialization/Deserialization of the RelationSlab
> - Integrating RelationSlab with SymbolCollector
> - Adding queries to index structures(memindex, dex, mergedindex etc.)
> - Surfacing those queries through typehierarchy request


Thanks. The proposed split looks good to me. I've posted the first patch here: 
https://reviews.llvm.org/D59407


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58880/new/

https://reviews.llvm.org/D58880



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked an inline comment as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/index/Index.h:25
 
+enum class RelationKind { Subtype };
+

One thing I'm wondering about is: would it be better to just use 
`clang::index::SymbolRole` here (which has `Relation___Of` entries that cover 
what we're interested in) instead of having our own enum?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Great, thanks for the reviews!

Could you commit the patch as well?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D58880#1422494 , @kadircet wrote:

> Also please have a look at D59083 , and make 
> sure it helps implement what you have in might and let me know if there's 
> anything missing.


I verified that with D59083  + D59354 
, I'm able to get my template specialization 
tests passing. Thanks for working on those!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58880/new/

https://reviews.llvm.org/D58880



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D59407#1430656 , @kadircet wrote:

> I believe it makes sense to deduplicate SymbolIDs for RelationSlab.
>  Up until now, we mostly had only one occurence of a SymbolID in a Slab, but 
> RelationSlab does not follow that assumption.


Just to make sure I understand, do you mean:

(A) When adding a `SymbolID` to an entry's value, check that it's not already 
there; or
(B) Try to conserve space by not storing `SymbolID`s directly in the entries, 
but storing an index into a separate list of unique `SymbolID`s.

If it's (B), how many bytes should the index be? Are the space gains worth the 
complexity, given that `SymbolID` is only 8 bytes to begin with? (As compared 
to say, the filenames in `Ref`, which can be much longer, making this sort of 
optimization more clearly worth it.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 7 inline comments as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/index/Index.h:43
+public:
+  using value_type = std::pair>;
+  using const_iterator = std::vector::const_iterator;

kadircet wrote:
> gribozavr wrote:
> > `struct Relation`?  And in the comments for it, please explain which way 
> > the relationship is directed (is the SymbolID in the key the subtype?  or 
> > is the SymbolID in the ArrayRef the subtype?).
> Ah exactly my thoughts, forget to mention this.
> 
> I believe current usage is the counter-intuitive one. For example, we will 
> most likely query with something like:
> `getRelations(SymbolID, baseOf)` to get all relations where `SymbolID` is 
> `baseOf` something else(which says get children of `SymbolID`)
> 
> So that this valueType can be read like, 
> ```
> `SymbolID` is `RelationKind` every `SymbolID inside array`
> ```
> WDYT?
The way I was thinking of it is that `getRelations(SymbolID, baseOf)` would 
return all the bases of `SymbolID`. However, the opposite interpretation is 
also reasonable, we just need to pick one and document it. I'm happy to go with 
your suggested one.



Comment at: clang-tools-extra/clangd/index/Index.h:59
+return sizeof(*this) + Arena.getTotalMemory() +
+   sizeof(value_type) * Relations.size();
+  }

kadircet wrote:
> use capacity instead of size
Note, `RefSlab::bytes()` (which I where I copied this from) uses `size()` as 
well. Should I change that too?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 191036.
nridge marked an inline comment as done.
nridge added a comment.
Herald added a subscriber: mgorny.

Address review comments, except for the deduplication which is still under 
discussion


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/Relation.cpp
  clang-tools-extra/clangd/index/Relation.h

Index: clang-tools-extra/clangd/index/Relation.h
===
--- /dev/null
+++ clang-tools-extra/clangd/index/Relation.h
@@ -0,0 +1,124 @@
+//===--- Ref.h ---*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_RELATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_RELATION_H
+
+#include "SymbolID.h"
+#include "SymbolLocation.h"
+#include "clang/Index/IndexSymbol.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/Support/StringSaver.h"
+#include "llvm/Support/raw_ostream.h"
+#include 
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+
+struct RelationKey {
+  SymbolID Symbol;
+  index::SymbolRole Kind;
+
+  bool operator==(const RelationKey &Other) const {
+return Symbol == Other.Symbol && Kind == Other.Kind;
+  }
+
+private:
+  friend llvm::hash_code hash_value(const RelationKey &Key) {
+return llvm::hash_combine(Key.Symbol, static_cast(Key.Kind));
+  }
+};
+
+class RelationSlab {
+public:
+  // Key.Symbol is Key.Kind of every symbol in Value.
+  // For example, if Key.Kind == SymbolRole::RelationChildOf,
+  // then Key.Symbol is the child of every symbol in Value (i.e. the symbols
+  // in Value are the base classes of Key.Symbol).
+  struct Relation {
+RelationKey Key;
+llvm::ArrayRef Value;
+  };
+  using value_type = std::pair>;
+  using const_iterator = std::vector::const_iterator;
+  using iterator = const_iterator;
+
+  RelationSlab() = default;
+  RelationSlab(RelationSlab &&Slab) = default;
+  RelationSlab &operator=(RelationSlab &&RHS) = default;
+
+  const_iterator begin() const { return Relations.begin(); }
+  const_iterator end() const { return Relations.end(); }
+  size_t size() const { return Relations.size(); }
+  size_t numRelations() const { return NumRelations; }
+  bool empty() const { return Relations.empty(); }
+
+  size_t bytes() const {
+return sizeof(*this) + Arena.getTotalMemory() +
+   sizeof(value_type) * Relations.capacity();
+  }
+
+  // RelationSlab::Builder is a mutable container that can 'freeze' to
+  // RelationSlab.
+  class Builder {
+  public:
+Builder() {}
+// Adds a relation to the slab.
+void insert(const RelationKey &Key, const SymbolID &S);
+// Consumes the builder to finalize the slab.
+RelationSlab build() &&;
+
+  private:
+llvm::BumpPtrAllocator Arena;
+llvm::DenseMap> Relations;
+  };
+
+private:
+  RelationSlab(std::vector Relations, llvm::BumpPtrAllocator Arena,
+   size_t NumRelations)
+  : Arena(std::move(Arena)), Relations(std::move(Relations)),
+NumRelations(NumRelations) {}
+
+  llvm::BumpPtrAllocator Arena;
+  std::vector Relations;
+  // Number of all relations.
+  size_t NumRelations = 0;
+};
+
+} // namespace clangd
+} // namespace clang
+
+namespace llvm {
+
+// Support RelationKeys as DenseMap keys.
+template <> struct DenseMapInfo {
+  static inline clang::clangd::RelationKey getEmptyKey() {
+return {DenseMapInfo::getEmptyKey(),
+clang::index::SymbolRole::RelationChildOf};
+  }
+
+  static inline clang::clangd::RelationKey getTombstoneKey() {
+return {DenseMapInfo::getTombstoneKey(),
+clang::index::SymbolRole::RelationChildOf};
+  }
+
+  static unsigned getHashValue(const clang::clangd::RelationKey &Key) {
+return hash_value(Key);
+  }
+
+  static bool isEqual(const clang::clangd::RelationKey &LHS,
+  const clang::clangd::RelationKey &RHS) {
+return LHS == RHS;
+  }
+};
+
+} // namespace llvm
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_RELATION_H
Index: clang-tools-extra/clangd/index/Relation.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/index/Relation.cpp
@@ -0,0 +1,35 @@
+//===--- Ref.cpp -*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 8 inline comments as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/index/Index.h:43
+public:
+  using value_type = std::pair>;
+  using const_iterator = std::vector::const_iterator;

kadircet wrote:
> nridge wrote:
> > kadircet wrote:
> > > gribozavr wrote:
> > > > `struct Relation`?  And in the comments for it, please explain which 
> > > > way the relationship is directed (is the SymbolID in the key the 
> > > > subtype?  or is the SymbolID in the ArrayRef the subtype?).
> > > Ah exactly my thoughts, forget to mention this.
> > > 
> > > I believe current usage is the counter-intuitive one. For example, we 
> > > will most likely query with something like:
> > > `getRelations(SymbolID, baseOf)` to get all relations where `SymbolID` is 
> > > `baseOf` something else(which says get children of `SymbolID`)
> > > 
> > > So that this valueType can be read like, 
> > > ```
> > > `SymbolID` is `RelationKind` every `SymbolID inside array`
> > > ```
> > > WDYT?
> > The way I was thinking of it is that `getRelations(SymbolID, baseOf)` would 
> > return all the bases of `SymbolID`. However, the opposite interpretation is 
> > also reasonable, we just need to pick one and document it. I'm happy to go 
> > with your suggested one.
> It looks like IndexingAPI is also using the interpretation I suggested, so 
> let's move with that one if you don't have any other concerns.
Already updated to this interpretation :)



Comment at: clang-tools-extra/clangd/index/Relation.h:45
+  // in Value are the base classes of Key.Symbol).
+  struct Relation {
+RelationKey Key;

gribozavr wrote:
> Lift it up into the `clang::clangd` namespace?  (like `Symbol` and `Ref`)
This comment made me realize that I haven't addressed your previous comment 
properly: I haven't changed `RelationSlab::value_type` from 
`std::pair>` to `Relation`.

I tried to make that change this time, and ran into a problem:

In the rest of the subtypes patch (D58880), one of the things I do is extend 
the `MemIndex` constructor so that, in addition to taking a symbol range and a 
ref range, it takes a relation range.

That constructor assumes that the elements of that range have members of some 
name - either `first` and `second` (as currently in D58880), or `Key` and 
`Value`.

However, that constructor has two call sites. In `MemIndex::build()`, we pass 
in the slabs themselves as the ranges. So, if we make this change, the field 
names for that call site will be `Key` and `Value`. However, for the second 
call site in `FileSymbols::buildIndex()`, the ranges that are passed in are 
`DenseMap`s, and therefore their elements' field names are necessarily `first` 
and `second`. The same constructor cannot therefore accept both ranges.

How do you propose we address this?

 * Scrap `struct Relation`, and keep `value_type` as `std::pair>`?
 * Keep `struct Relation`, but make its fields named `first` and `second`?
 * Split the constructor of `MemIndex` into two constructors, to accomodate 
both sets of field names?
 * Something else?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 2 inline comments as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/index/Relation.h:1
+//===--- Ref.h ---*- 
C++-*-===//
+//

gribozavr wrote:
> gribozavr wrote:
> > "--- Relation.h "
> Not done?
(Sorry, I have these comments addressed locally, was just waiting for the 
resolution of the remaining issue before uploading.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 191886.
nridge added a comment.

Scrapped 'struct Relation' and addressed other comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/Relation.cpp
  clang-tools-extra/clangd/index/Relation.h

Index: clang-tools-extra/clangd/index/Relation.h
===
--- /dev/null
+++ clang-tools-extra/clangd/index/Relation.h
@@ -0,0 +1,120 @@
+//===--- Relation.h --*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_RELATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_RELATION_H
+
+#include "SymbolID.h"
+#include "SymbolLocation.h"
+#include "clang/Index/IndexSymbol.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/Support/StringSaver.h"
+#include "llvm/Support/raw_ostream.h"
+#include 
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+
+struct RelationKey {
+  SymbolID Symbol;
+  index::SymbolRole Kind;
+
+  bool operator==(const RelationKey &Other) const {
+return Symbol == Other.Symbol && Kind == Other.Kind;
+  }
+
+private:
+  friend llvm::hash_code hash_value(const RelationKey &Key) {
+return llvm::hash_combine(Key.Symbol, static_cast(Key.Kind));
+  }
+};
+
+class RelationSlab {
+public:
+  /// The interpretation of a pair (Key, Value) is:
+  /// " is  of every symbol in ".
+  /// For example, if Key.Kind == SymbolRole::RelationChildOf,
+  /// then Key.Symbol is the child of every symbol in Value (i.e. the symbols
+  /// in Value are the base classes of Key.Symbol).
+  using value_type = std::pair>;
+  using const_iterator = std::vector::const_iterator;
+  using iterator = const_iterator;
+
+  RelationSlab() = default;
+  RelationSlab(RelationSlab &&Slab) = default;
+  RelationSlab &operator=(RelationSlab &&RHS) = default;
+
+  const_iterator begin() const { return Relations.begin(); }
+  const_iterator end() const { return Relations.end(); }
+  size_t size() const { return Relations.size(); }
+  size_t numRelations() const { return NumRelations; }
+  bool empty() const { return Relations.empty(); }
+
+  size_t bytes() const {
+return sizeof(*this) + Arena.getTotalMemory() +
+   sizeof(value_type) * Relations.capacity();
+  }
+
+  /// A mutable container that can 'freeze' to RelationSlab.
+  class Builder {
+  public:
+Builder() {}
+/// Adds a relation to the slab.
+void insert(const RelationKey &Key, const SymbolID &S);
+/// Consumes the builder to finalize the slab.
+RelationSlab build() &&;
+
+  private:
+llvm::BumpPtrAllocator Arena;
+llvm::DenseMap> Relations;
+  };
+
+private:
+  RelationSlab(std::vector Relations, llvm::BumpPtrAllocator Arena,
+   size_t NumRelations)
+  : Arena(std::move(Arena)), Relations(std::move(Relations)),
+NumRelations(NumRelations) {}
+
+  llvm::BumpPtrAllocator Arena;
+  std::vector Relations;
+  // Number of all relations.
+  size_t NumRelations = 0;
+};
+
+} // namespace clangd
+} // namespace clang
+
+namespace llvm {
+
+// Support RelationKeys as DenseMap keys.
+template <> struct DenseMapInfo {
+  static inline clang::clangd::RelationKey getEmptyKey() {
+return {DenseMapInfo::getEmptyKey(),
+clang::index::SymbolRole::RelationChildOf};
+  }
+
+  static inline clang::clangd::RelationKey getTombstoneKey() {
+return {DenseMapInfo::getTombstoneKey(),
+clang::index::SymbolRole::RelationChildOf};
+  }
+
+  static unsigned getHashValue(const clang::clangd::RelationKey &Key) {
+return hash_value(Key);
+  }
+
+  static bool isEqual(const clang::clangd::RelationKey &LHS,
+  const clang::clangd::RelationKey &RHS) {
+return LHS == RHS;
+  }
+};
+
+} // namespace llvm
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_RELATION_H
Index: clang-tools-extra/clangd/index/Relation.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/index/Relation.cpp
@@ -0,0 +1,35 @@
+//===--- Relation.cpp *- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Relation.h"
+
+namespace clang {
+namespace clangd {
+
+void RelationSlab::Builder::insert(const

[PATCH] D59756: [clangd] Support dependent bases in type hierarchy

[Nathan Ridge via Phabricator via cfe-commits]

nridge created this revision.
nridge added a reviewer: sammccall.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ioeric, 
ilya-biryukov.
Herald added a project: clang.

Dependent bases are handled heuristically, by replacing them with the class
template that they are a specialization of, where possible. Care is taken
to avoid infinite recursion.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59756

Files:
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp

Index: clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
===
--- clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
+++ clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
@@ -291,9 +291,7 @@
   EXPECT_THAT(typeParents(ChildSpec), ElementsAre(Parent));
 }
 
-// This is disabled for now, because support for dependent bases
-// requires additional measures to avoid infinite recursion.
-TEST(DISABLED_TypeParents, DependentBase) {
+TEST(TypeParents, DependentBase) {
   Annotations Source(R"cpp(
 template 
 struct Parent {};
@@ -386,7 +384,7 @@
 TEST(TypeHierarchy, RecursiveHierarchy1) {
   Annotations Source(R"cpp(
   template 
-  struct S : S {};
+  struct $SDef[[S]] : S {};
 
   S^<0> s;
   )cpp");
@@ -402,14 +400,17 @@
   llvm::Optional Result = getTypeHierarchy(
   AST, Source.points()[0], 0, TypeHierarchyDirection::Parents);
   ASSERT_TRUE(bool(Result));
-  EXPECT_THAT(*Result,
-  AllOf(WithName("S"), WithKind(SymbolKind::Struct), Parents()));
+  EXPECT_THAT(
+  *Result,
+  AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("SDef")), Parents();
 }
 
 TEST(TypeHierarchy, RecursiveHierarchy2) {
   Annotations Source(R"cpp(
   template 
-  struct S : S {};
+  struct $SDef[[S]] : S {};
 
   template <>
   struct S<0>{};
@@ -426,14 +427,17 @@
   llvm::Optional Result = getTypeHierarchy(
   AST, Source.points()[0], 0, TypeHierarchyDirection::Parents);
   ASSERT_TRUE(bool(Result));
-  EXPECT_THAT(*Result,
-  AllOf(WithName("S"), WithKind(SymbolKind::Struct), Parents()));
+  EXPECT_THAT(
+  *Result,
+  AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("SDef")), Parents();
 }
 
 TEST(TypeHierarchy, RecursiveHierarchy3) {
   Annotations Source(R"cpp(
   template 
-  struct S : S {};
+  struct $SDef[[S]] : S {};
 
   template <>
   struct S<0>{};
@@ -453,8 +457,11 @@
   llvm::Optional Result = getTypeHierarchy(
   AST, Source.points()[0], 0, TypeHierarchyDirection::Parents);
   ASSERT_TRUE(bool(Result));
-  EXPECT_THAT(*Result,
-  AllOf(WithName("S"), WithKind(SymbolKind::Struct), Parents()));
+  EXPECT_THAT(
+  *Result,
+  AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("SDef")), Parents();
 }
 
 SymbolID findSymbolIDByName(llvm::StringRef Name, SymbolIndex *Index) {
@@ -599,8 +606,7 @@
   EXPECT_THAT(collectSubtypes(Parent, Index.get()), ElementsAre(ChildSpec));
 }
 
-// Disabled for the same reason as TypeParents.DependentBase.
-TEST(DISABLED_Subtypes, DependentBase) {
+TEST(Subtypes, DependentBase) {
   Annotations Source(R"cpp(
 template 
 struct Parent {};
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -935,20 +935,40 @@
});
 }
 
-static Optional getTypeAncestors(const CXXRecordDecl &CXXRD,
-ASTContext &ASTCtx) {
+using RecursionProtectionSet = llvm::SmallSet;
+
+static Optional
+getTypeAncestors(const CXXRecordDecl &CXXRD, ASTContext &ASTCtx,
+ RecursionProtectionSet &RPSet) {
   Optional Result = declToTypeHierarchyItem(ASTCtx, CXXRD);
   if (!Result)
 return Result;
 
   Result->parents.emplace();
 
+  // typeParents() will replace dependent template specializations
+  // with their class template, so to avoid infinite recursion for
+  // certain types of hierarchies, keep the templates encountered
+  // along the parent chain in a set, and stop the recursion if one
+  // starts to repeat.
+  auto *Pattern = CXXRD.getDescribedTemplate() ? &CXXRD : nullptr;
+  if (Pattern) {
+if (!RPSet.insert(Pattern).second) {
+  return Result;
+}
+  }
+
   for (const CXXRecordDecl *ParentDecl : typeParents(&CXXRD)) {
 if (Optional ParentSym =
-getTypeAncestors(*ParentDecl, ASTCtx)) {
+getTypeAncestors(*ParentDecl, ASTCtx, RPSet)) {
   Result->parents->emplace_ba

[PATCH] D59756: [clangd] Support dependent bases in type hierarchy

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

This patch aims to implement the more proper solution suggested here 
.

I couldn't actually use `CXXRecordDecl->getTemplateInstantiationPattern()` as 
suggested, because the base-specifiers resolve to types, not declarations, but 
I believe what I did here implements the same intention (please let me know if 
I misunderstood).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59756/new/

https://reviews.llvm.org/D59756



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

As this is the first of a series of patches adding support for relations, and 
then building type hierarchy subtypes on top (as discussed here 
), how should we go about landing this 
-- should we land each patch in the series as soon as it's ready, or should we 
wait to land them all together?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Ok, cool. In that case, I think this patch is ready to be committed, and would 
appreciate it if someone could commit it. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

@sammccall, thank you for having a look at this.

I have no objection to revising the data model if there's agreement on a better 
one.

In D59407#1446464 , @sammccall wrote:

> - I don't think we yet know what the more resource-critical (denser) 
> relations and queries are, so it's unclear what to optimize for


Based on a brief mental survey of C++ IDE features I'm familiar with, I can 
think of the following additional uses of the relations capability:

- A call hierarchy feature (which is also proposed for LSP 
, with client 
 and server 
 
implementation efforts) would need every caller-callee relationship to be 
recorded in the index (`RelationCalledBy`).
- Given a virtual method declaration, a user may want to see a list of 
implementations (overriders) and navigate to one or more of them. This would 
need every overrider relationship to be recorded in the index 
(`RelationOverrideOf`).

Intuitively, it seems like `RelationOverrideOf` would be slightly denser than 
`RelationChildOf` (though not by much), while `RelationCalledBy` would be 
significantly denser. In terms of queries, I believe the key for lookups for 
both of the above would be a (subject, predicate) pair, just like for subtypes.

Does that change your analysis at all?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57560: [clangd] Link libclangAST into clangd tool

[Nathan Ridge via Phabricator via cfe-commits]

nridge abandoned this revision.
nridge added a comment.
Herald added a subscriber: cfe-commits.

Abandoning as this has been fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57560/new/

https://reviews.llvm.org/D57560



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60040: [clangd] Use capacity() instead of size() in RefSlab::bytes()

[Nathan Ridge via Phabricator via cfe-commits]

nridge created this revision.
nridge added a reviewer: gribozavr.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ioeric, ilya-biryukov.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60040

Files:
  clang-tools-extra/clangd/index/Ref.h


Index: clang-tools-extra/clangd/index/Ref.h
===
--- clang-tools-extra/clangd/index/Ref.h
+++ clang-tools-extra/clangd/index/Ref.h
@@ -86,7 +86,7 @@
 
   size_t bytes() const {
 return sizeof(*this) + Arena.getTotalMemory() +
-   sizeof(value_type) * Refs.size();
+   sizeof(value_type) * Refs.capacity();
   }
 
   /// RefSlab::Builder is a mutable container that can 'freeze' to RefSlab.


Index: clang-tools-extra/clangd/index/Ref.h
===
--- clang-tools-extra/clangd/index/Ref.h
+++ clang-tools-extra/clangd/index/Ref.h
@@ -86,7 +86,7 @@
 
   size_t bytes() const {
 return sizeof(*this) + Arena.getTotalMemory() +
-   sizeof(value_type) * Refs.size();
+   sizeof(value_type) * Refs.capacity();
   }
 
   /// RefSlab::Builder is a mutable container that can 'freeze' to RefSlab.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60040: [clangd] Use capacity() instead of size() in RefSlab::bytes()

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

This was suggested in this comment 
.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60040/new/

https://reviews.llvm.org/D60040



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60040: [clangd] Use capacity() instead of size() in RefSlab::bytes()

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

I do not.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60040/new/

https://reviews.llvm.org/D60040



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge requested review of this revision.
nridge added a reviewer: sammccall.
nridge added a comment.

I guess I should clear the "Accepted" status until we settle the question of 
the data model.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D59407#1456394 , @sammccall wrote:

> One thing that strikes me here is that this case is very similar to our 
> existing Ref data - it's basically a subset, but with a symbolid payload 
> instead of location. We could consider just adding the SymbolID to Refs - 
> it'd blow up the size of that by 50%, but we may not do much better with some 
> other representation, and it would avoid adding any new complexity.


Note that this was considered and rejected earlier (see here 
 and here 
), though that discussion did not 
consider denser relations like callgraph.

Given the additional information and perspectives from the conversation here, I 
have two suggestions for potential ways forward:

**Approach 1: Add SymbolID to Refs**

- This would be initially wasteful when only subtypes use it, but if we then 
build callgraph on top of it as well it will become significantly less wasteful.
- This has the benefit that we don't have duplicate information for 
find-references and callgraph (they both use Refs).
- This approach probably adds the least amount of complexity overall.

**Approach 2: Add a RelationSlab storing (subject, predicate, object) triples, 
intended for sparse relations**

- This would allow us to implement features that require sparse relations, such 
as subtypes and overrides, without any significant increase to index size.
- If we later want to add dense relations like callgraph, we'd use a different 
mechanism for them (possibly by adding SymbolID to Refs as in approach 1).
- If we do end up adding SymbolID to Refs for callgraph, and want to start 
using that for sparse relations too, we can rip out RelationSlab.
- This also adds relatively little complexity, though slightly more than 
approach 1, and we are throwing away some code if we end up adding SymbolID to 
Refs eventually.

Any thoughts on which of the approaches to take, or something else altogether? 
It would be good to have confidence that the chosen approach will pass code 
review before I spend time implementing it :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59756: [clangd] Support dependent bases in type hierarchy

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 194002.
nridge marked 6 inline comments as done.
nridge added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59756/new/

https://reviews.llvm.org/D59756

Files:
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp

Index: clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
===
--- clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
+++ clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
@@ -291,9 +291,7 @@
   EXPECT_THAT(typeParents(ChildSpec), ElementsAre(Parent));
 }
 
-// This is disabled for now, because support for dependent bases
-// requires additional measures to avoid infinite recursion.
-TEST(DISABLED_TypeParents, DependentBase) {
+TEST(TypeParents, DependentBase) {
   Annotations Source(R"cpp(
 template 
 struct Parent {};
@@ -383,10 +381,10 @@
   }
 }
 
-TEST(TypeHierarchy, RecursiveHierarchy1) {
+TEST(TypeHierarchy, RecursiveHierarchyUnbounded) {
   Annotations Source(R"cpp(
   template 
-  struct S : S {};
+  struct $SDef[[S]] : S {};
 
   S^<0> s;
   )cpp");
@@ -399,62 +397,57 @@
   ASSERT_TRUE(!AST.getDiagnostics().empty());
 
   // Make sure getTypeHierarchy() doesn't get into an infinite recursion.
+  // FIXME(nridge): It would be preferable if the type hierarchy gave us type
+  // names (e.g. "S<0>" for the child and "S<1>" for the parent) rather than
+  // template names (e.g. "S").
   llvm::Optional Result = getTypeHierarchy(
   AST, Source.points()[0], 0, TypeHierarchyDirection::Parents);
   ASSERT_TRUE(bool(Result));
-  EXPECT_THAT(*Result,
-  AllOf(WithName("S"), WithKind(SymbolKind::Struct), Parents()));
+  EXPECT_THAT(
+  *Result,
+  AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("SDef")), Parents();
 }
 
-TEST(TypeHierarchy, RecursiveHierarchy2) {
+TEST(TypeHierarchy, RecursiveHierarchyBounded) {
   Annotations Source(R"cpp(
   template 
-  struct S : S {};
+  struct $SDef[[S]] : S {};
 
   template <>
   struct S<0>{};
 
-  S^<2> s;
-  )cpp");
-
-  TestTU TU = TestTU::withCode(Source.code());
-  auto AST = TU.build();
-
-  ASSERT_TRUE(AST.getDiagnostics().empty());
-
-  // Make sure getTypeHierarchy() doesn't get into an infinite recursion.
-  llvm::Optional Result = getTypeHierarchy(
-  AST, Source.points()[0], 0, TypeHierarchyDirection::Parents);
-  ASSERT_TRUE(bool(Result));
-  EXPECT_THAT(*Result,
-  AllOf(WithName("S"), WithKind(SymbolKind::Struct), Parents()));
-}
-
-TEST(TypeHierarchy, RecursiveHierarchy3) {
-  Annotations Source(R"cpp(
-  template 
-  struct S : S {};
-
-  template <>
-  struct S<0>{};
+  S$SRefConcrete^<2> s;
 
   template 
   struct Foo {
-S^ s;
-  };
-  )cpp");
+S$SRefDependent^ s;
+  };)cpp");
 
   TestTU TU = TestTU::withCode(Source.code());
   auto AST = TU.build();
 
   ASSERT_TRUE(AST.getDiagnostics().empty());
 
-  // Make sure getTypeHierarchy() doesn't get into an infinite recursion.
+  // Make sure getTypeHierarchy() doesn't get into an infinite recursion
+  // for either a concrete starting point or a dependent starting point.
   llvm::Optional Result = getTypeHierarchy(
-  AST, Source.points()[0], 0, TypeHierarchyDirection::Parents);
+  AST, Source.point("SRefConcrete"), 0, TypeHierarchyDirection::Parents);
+  ASSERT_TRUE(bool(Result));
+  EXPECT_THAT(
+  *Result,
+  AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("SDef")), Parents();
+  Result = getTypeHierarchy(AST, Source.point("SRefDependent"), 0,
+TypeHierarchyDirection::Parents);
   ASSERT_TRUE(bool(Result));
-  EXPECT_THAT(*Result,
-  AllOf(WithName("S"), WithKind(SymbolKind::Struct), Parents()));
+  EXPECT_THAT(
+  *Result,
+  AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("SDef")), Parents();
 }
 
 } // namespace
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -876,21 +876,40 @@
   return THI;
 }
 
-static Optional getTypeAncestors(const CXXRecordDecl &CXXRD,
-ASTContext &ASTCtx) {
+using RecursionProtectionSet = llvm::SmallSet;
+
+static Optional
+getTypeAncestors(const CXXRecordDecl &CXXRD, ASTContext &ASTCtx,
+ RecursionProtectionSet &RPSet) {
   Optional Result = declToTypeHierarchyItem(ASTCtx, CXXRD);
   if (!Result

[PATCH] D59756: [clangd] Support dependent bases in type hierarchy

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked an inline comment as done.
nridge added inline comments.



Comment at: clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp:405
+  *Result,
+  AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("S"), WithKind(SymbolKind::Struct),

sammccall wrote:
> Sorry, I realize this isn't related to this patch, but I didn't see the final 
> form of the previous one.
> 
> This should be `WithName("S<0>"), ... Parents(AllOf(WithName("S<1>")), ...)`. 
> S is the name of the template, not the name of the type.
> 
> Can you add a fixme?
(I looked into what it would take to fix this, and it seems like we need 
D59639, so I'm going to wait for that.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59756/new/

https://reviews.llvm.org/D59756



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59756: [clangd] Support dependent bases in type hierarchy

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked an inline comment as done.
nridge added inline comments.



Comment at: clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp:405
+  *Result,
+  AllOf(WithName("S"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("S"), WithKind(SymbolKind::Struct),

sammccall wrote:
> nridge wrote:
> > sammccall wrote:
> > > Sorry, I realize this isn't related to this patch, but I didn't see the 
> > > final form of the previous one.
> > > 
> > > This should be `WithName("S<0>"), ... Parents(AllOf(WithName("S<1>")), 
> > > ...)`. S is the name of the template, not the name of the type.
> > > 
> > > Can you add a fixme?
> > (I looked into what it would take to fix this, and it seems like we need 
> > D59639, so I'm going to wait for that.)
> AFAICS D59639 is something subtly different - it prints the args of 
> *specializations* as written in the source code, not instantiations. i.e. if 
> you try to use this for `vector`, it won't work as there's no 
> specialization, you'll get `vector` instead.
I had another look, and you're right; I didn't appreciate that distinction.

I filed [an issue](https://github.com/clangd/clangd/issues/31) to track 
implementing your suggestion. I plan to work on it.

In the meantime, could we get this patch committed?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59756/new/

https://reviews.llvm.org/D59756



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge created this revision.
nridge added a reviewer: hokein.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ioeric.
Herald added a project: clang.

This partially fixes https://bugs.llvm.org/show_bug.cgi?id=41218.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60953

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clangd/ClangdUnit.cpp

Index: clang-tools-extra/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/clangd/ClangdUnit.cpp
+++ clang-tools-extra/clangd/ClangdUnit.cpp
@@ -237,6 +237,39 @@
   const LangOptions &LangOpts;
 };
 
+// A wrapper around StoreDiags to handle suppression comments for
+// clang-tidy diagnostics (and possibly other clang-tidy customizations in the
+// future).
+class ClangdDiagnosticConsumer : public StoreDiags {
+public:
+  void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+const clang::Diagnostic &Info) override {
+if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
+  return;
+
+if (CTContext) {
+  bool isClangTidyDiag = !CTContext->getCheckName(Info.getID()).empty();
+  if (isClangTidyDiag &&
+  tidy::ShouldSuppressDiagnostic(DiagLevel, Info, *CTContext)) {
+// Ignored a warning, should ignore related notes as well
+LastErrorWasIgnored = true;
+return;
+  }
+}
+
+LastErrorWasIgnored = false;
+StoreDiags::HandleDiagnostic(DiagLevel, Info);
+  }
+
+  void setClangTidyContext(tidy::ClangTidyContext *CTContext) {
+this->CTContext = CTContext;
+  }
+
+private:
+  bool LastErrorWasIgnored = false;
+  tidy::ClangTidyContext *CTContext = nullptr;
+};
+
 } // namespace
 
 void dumpAST(ParsedAST &AST, llvm::raw_ostream &OS) {
@@ -256,7 +289,7 @@
   const PrecompiledPreamble *PreamblePCH =
   Preamble ? &Preamble->Preamble : nullptr;
 
-  StoreDiags ASTDiags;
+  ClangdDiagnosticConsumer ASTDiags;
   std::string Content = Buffer->getBuffer();
 
   auto Clang = prepareCompilerInstance(std::move(CI), PreamblePCH,
@@ -294,6 +327,7 @@
 CTContext->setASTContext(&Clang->getASTContext());
 CTContext->setCurrentFile(MainInput.getFile());
 CTFactories.createChecks(CTContext.getPointer(), CTChecks);
+ASTDiags.setClangTidyContext(CTContext.getPointer());
 Preprocessor *PP = &Clang->getPreprocessor();
 for (const auto &Check : CTChecks) {
   // FIXME: the PP callbacks skip the entire preamble.
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -217,6 +217,17 @@
   bool AllowEnablingAnalyzerAlphaCheckers;
 };
 
+/// Check whether a given diagnostic should be suppressed due to the presence
+/// of a "NOLINT" suppression comment.
+/// This is exposed so that other tools that present clang-tidy diagnostics
+/// (such as clangd) can respect the same suppression rules as clang-tidy.
+/// This does not handle suppression of notes following a suppressed diagnostic;
+/// that is left to the caller is it requires maintaining state in between calls
+/// to this function.
+bool ShouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
+  const Diagnostic &Info,
+  ClangTidyContext &Context);
+
 /// \brief A diagnostic consumer that turns each \c Diagnostic into a
 /// \c SourceManager-independent \c ClangTidyError.
 //
@@ -246,7 +257,7 @@
 
   /// \brief Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter
   /// according to the diagnostic \p Location.
-  void checkFilters(SourceLocation Location, const SourceManager& Sources);
+  void checkFilters(SourceLocation Location, const SourceManager &Sources);
   bool passesLineFilter(StringRef FileName, unsigned LineNumber) const;
 
   ClangTidyContext &Context;
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -160,11 +160,13 @@
 
   bool contains(StringRef S) {
 switch (auto &Result = Cache[S]) {
-  case Yes: return true;
-  case No: return false;
-  case None:
-Result = Globs.contains(S) ? Yes : No;
-return Result == Yes;
+case Yes:
+  return true;
+case No:
+  return false;
+case None:
+  Result = Globs.contains(S) ? Yes : No;
+  return Result == Yes;
 }
 llvm_unreachable("invalid enum");
   }
@@ -381,16 +383,29 @@
   return false;
 }
 
+namespace clang {
+namespace tidy {
+
+bool ShouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLev

[PATCH] D59756: [clangd] Support dependent bases in type hierarchy

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Ping - could this be committed please?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59756/new/

https://reviews.llvm.org/D59756



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Hi, any update here? I would appreciate some guidance so I can move forward 
with this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60954: [clangd] Test case demonstrating variable template issue

[Nathan Ridge via Phabricator via cfe-commits]

nridge created this revision.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ioeric, ilya-biryukov.
Herald added a project: clang.

This is not meant to be committed, it's meant to demonstrate an
issue with using RecursiveASTVisitor with TestTU on code that contains a
top-level variable template.

The variable template declaration is never visited, for reasons I don't
fully understand.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60954

Files:
  clang-tools-extra/unittests/clangd/TestTU.cpp


Index: clang-tools-extra/unittests/clangd/TestTU.cpp
===
--- clang-tools-extra/unittests/clangd/TestTU.cpp
+++ clang-tools-extra/unittests/clangd/TestTU.cpp
@@ -141,5 +141,24 @@
   });
 }
 
+class TestVisitor : public RecursiveASTVisitor {
+public:
+  bool VisitVarTemplateDecl(VarTemplateDecl *VTD) {
+seenVarTemplateDecl = true;
+return true;
+  }
+
+  bool seenVarTemplateDecl = false;
+};
+
+TEST(TestTU, VariableTemplate) {
+  TestTU TU = TestTU::withCode("template  int x;");
+  auto AST = TU.build();
+  EXPECT_TRUE(AST.getDiagnostics().empty());
+  TestVisitor Visitor;
+  Visitor.TraverseAST(AST.getASTContext());
+  EXPECT_TRUE(Visitor.seenVarTemplateDecl);
+}
+
 } // namespace clangd
 } // namespace clang


Index: clang-tools-extra/unittests/clangd/TestTU.cpp
===
--- clang-tools-extra/unittests/clangd/TestTU.cpp
+++ clang-tools-extra/unittests/clangd/TestTU.cpp
@@ -141,5 +141,24 @@
   });
 }
 
+class TestVisitor : public RecursiveASTVisitor {
+public:
+  bool VisitVarTemplateDecl(VarTemplateDecl *VTD) {
+seenVarTemplateDecl = true;
+return true;
+  }
+
+  bool seenVarTemplateDecl = false;
+};
+
+TEST(TestTU, VariableTemplate) {
+  TestTU TU = TestTU::withCode("template  int x;");
+  auto AST = TU.build();
+  EXPECT_TRUE(AST.getDiagnostics().empty());
+  TestVisitor Visitor;
+  Visitor.TraverseAST(AST.getASTContext());
+  EXPECT_TRUE(Visitor.seenVarTemplateDecl);
+}
+
 } // namespace clangd
 } // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60954: [clangd] Test case demonstrating variable template issue

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Please see further details in this clangd-dev post 
.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60954/new/

https://reviews.llvm.org/D60954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59756: [clangd] Support dependent bases in type hierarchy

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59756/new/

https://reviews.llvm.org/D59756



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge planned changes to this revision.
nridge added a comment.

In D59407#1478794 , @sammccall wrote:

> So if you can stomach it, I think
>
> > **Approach 2: Add a RelationSlab storing (subject, predicate, object) 
> > triples, intended for sparse relations***
>
> is certainly fine with us (having spoken with @kadircet @ilya-biryukov 
> @sammccall @gribozavr - @hokein is on vacation but not nearly as stubborn as 
> I am!)


Yep, I'm happy to move forward with that approach. Thanks for the guidance!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge edited reviewers, added: ilya-biryukov; removed: hokein.
nridge added a comment.

Changing reviewers as I understand @hokein is on vacation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 197040.
nridge marked 3 inline comments as done.
nridge added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clangd/ClangdUnit.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h

Index: clang-tools-extra/clangd/Diagnostics.h
===
--- clang-tools-extra/clangd/Diagnostics.h
+++ clang-tools-extra/clangd/Diagnostics.h
@@ -113,16 +113,23 @@
 
   using DiagFixer = std::function(DiagnosticsEngine::Level,
const clang::Diagnostic &)>;
+  using DiagFilter =
+  std::function;
   /// If set, possibly adds fixes for diagnostics using \p Fixer.
   void contributeFixes(DiagFixer Fixer) { this->Fixer = Fixer; }
+  /// If set, ignore diagnostics for which \p Filter returns false.
+  void filterDiagnostics(DiagFilter Filter) { this->Filter = Filter; }
 
 private:
   void flushLastDiag();
 
   DiagFixer Fixer = nullptr;
+  DiagFilter Filter = nullptr;
   std::vector Output;
   llvm::Optional LangOpts;
   llvm::Optional LastDiag;
+  bool LastErrorWasIgnored = false;
 };
 
 } // namespace clangd
Index: clang-tools-extra/clangd/Diagnostics.cpp
===
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -311,8 +311,17 @@
 return;
   }
 
+  if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
+return;
+
   bool InsideMainFile = isInsideMainFile(Info);
 
+  if (Filter && !Filter(DiagLevel, Info, InsideMainFile)) {
+LastErrorWasIgnored = true;
+return;
+  }
+  LastErrorWasIgnored = false;
+
   auto FillDiagBase = [&](DiagBase &D) {
 D.Range = diagnosticRange(Info, *LangOpts);
 llvm::SmallString<64> Message;
Index: clang-tools-extra/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/clangd/ClangdUnit.cpp
+++ clang-tools-extra/clangd/ClangdUnit.cpp
@@ -237,6 +237,40 @@
   const LangOptions &LangOpts;
 };
 
+// A wrapper around StoreDiags to handle suppression comments for
+// clang-tidy diagnostics (and possibly other clang-tidy customizations in the
+// future).
+class ClangdDiagnosticConsumer : public StoreDiags {
+public:
+  ClangdDiagnosticConsumer() {
+filterDiagnostics([this](DiagnosticsEngine::Level DiagLevel,
+ const clang::Diagnostic &Info,
+ bool IsInsideMainFile) {
+  if (CTContext) {
+bool isClangTidyDiag = !CTContext->getCheckName(Info.getID()).empty();
+// Skip the ShouldSuppressDiagnostic check for diagnostics not in
+// the main file, because we don't want that function to query the
+// source buffer for preamble files. For the same reason, we ask
+// ShouldSuppressDiagnostic not to follow macro expansions, since those
+// might take us into a preamble file as well.
+if (isClangTidyDiag && IsInsideMainFile &&
+tidy::ShouldSuppressDiagnostic(DiagLevel, Info, *CTContext,
+   /* CheckMacroExpansion = */ false)) {
+  return false;
+}
+  }
+  return true;
+});
+  }
+
+  void setClangTidyContext(tidy::ClangTidyContext *CTContext) {
+this->CTContext = CTContext;
+  }
+
+private:
+  tidy::ClangTidyContext *CTContext = nullptr;
+};
+
 } // namespace
 
 void dumpAST(ParsedAST &AST, llvm::raw_ostream &OS) {
@@ -256,7 +290,7 @@
   const PrecompiledPreamble *PreamblePCH =
   Preamble ? &Preamble->Preamble : nullptr;
 
-  StoreDiags ASTDiags;
+  ClangdDiagnosticConsumer ASTDiags;
   std::string Content = Buffer->getBuffer();
 
   auto Clang = prepareCompilerInstance(std::move(CI), PreamblePCH,
@@ -294,6 +328,7 @@
 CTContext->setASTContext(&Clang->getASTContext());
 CTContext->setCurrentFile(MainInput.getFile());
 CTFactories.createChecks(CTContext.getPointer(), CTChecks);
+ASTDiags.setClangTidyContext(CTContext.getPointer());
 Preprocessor *PP = &Clang->getPreprocessor();
 for (const auto &Check : CTChecks) {
   // FIXME: the PP callbacks skip the entire preamble.
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -217,6 +217,23 @@
   bool AllowEnablingAnalyzerAlphaCheckers;
 };
 
+/// Check whether a given diagnostic should be suppressed due to the presence
+/// of a "NOLINT" suppression comment.
+/// This is exposed so that other to

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:395
+ DiagLevel != DiagnosticsEngine::Fatal &&
+ LineIsMarkedWithNOLINTinMacro(Info.getSourceManager(),
+   Info.getLocation(), Info.getID(),

sammccall wrote:
> There may be a trap here for clangd.
> 
> When building an AST with a preamble (which is when we run clang-tidy 
> checks), the source code of the main file is mapped in the SourceManager as 
> usual, but headers are not.
> An attempt to get the buffer results in the SourceManager reading the file 
> from disk. If the content has changed then all sorts of invariants break and 
> clang will typically crash.
> 
> @ilya-biryukov knows the details here better than me.
> 
> Of course this is only true for trying to read from header file locations, 
> and we only run clang-tidy over main-file-decls. But:
>  - clang-tidy checks can emit diagnostics anywhere, even outside the AST 
> range they run over directly
>  - while StoreDiags does filter out diagnostics that aren't in the main file, 
> this is pretty nuanced (e.g. a note in the main file is sufficient to 
> include) and this logic runs after the filtering added by this patch
>  - the way that LineIsMarkedWithNOLINTinMacro loops suggests that even if the 
> diagnostic is in the main-file, we'll look for NOLINT in other files
> 
> Maybe this means we can't accurately calculate suppression for clang-tidy 
> warnings in macro locations, or outside the main file. I think *always* 
> filtering these out (on the clangd side) might be OK.
Thanks for pointing this out! This should be addressed in the updated patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41911: [clangd] Include scanner that finds compile commands for headers.

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.
Herald added subscribers: jfb, kadircet, arphaman, jkorous, MaskRay.
Herald added a project: clang.

Are there are plans to move forward with this sort of an include scanning 
approach?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D41911/new/

https://reviews.llvm.org/D41911



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.
Herald added a project: LLVM.

This commit is causing clangd to fail to link, with errors like this:

  
tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/SwapIfBranches.cpp.o:SwapIfBranches.cpp:function
 clang::RecursiveASTVisitor::TraverseParmVarDecl(clang::ParmVarDecl*): 
error: undefined reference to 'clang::ParmVarDecl::hasDefaultArg() const'

(and dozens more that are similar).


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56611/new/

https://reviews.llvm.org/D56611



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D56611#1379876 , @sammccall wrote:

> Hi Nathan,
>  What platform is this on? Not seeing it on the buildbots.
>  Anything unusual in build setup (standalone build, building with shared 
> libraries, etc)?


I'm on Linux, building with shared libraries. Not sure what standalone means in 
this context, I'm building the monorepo with `-DLLVM_ENABLE_PROJECTS="clang"`.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56611/new/

https://reviews.llvm.org/D56611



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

The complete commands that's failing is:

  /usr/bin/clang++-8  -fPIC -fvisibility-inlines-hidden -Werror=date-time 
-Werror=unguarded-availability-new -std=c++11 -Wall -Wextra 
-Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers 
-pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default 
-Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor 
-Wstring-conversion -fdiagnostics-color -fno-common -Woverloaded-virtual 
-Wno-nested-anon-types -O0 -g3  -fuse-ld=gold -Wl,-allow-shlib-undefined 
tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/SwapIfBranches.cpp.o
 tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o  -o 
bin/clangd  -Wl,-rpath,"\$ORIGIN/../lib" lib/libLLVMSupport.so.9svn -lpthread 
lib/libclangBasic.so.9svn lib/libclangTidy.so.9svn lib/libclangDaemon.so.9svn 
lib/libclangFormat.so.9svn lib/libclangFrontend.so.9svn 
lib/libclangSema.so.9svn lib/libclangTooling.so.9svn 
lib/libclangToolingCore.so.9svn


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56611/new/

https://reviews.llvm.org/D56611



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Fix here: https://reviews.llvm.org/D57560


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56611/new/

https://reviews.llvm.org/D56611



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 184984.
nridge edited the summary of this revision.
nridge added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This completes the implementation of the revised spec (for supertypes only)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -25,9 +25,11 @@
 namespace {
 
 using testing::ElementsAre;
+using testing::Eq;
 using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +41,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(&TypeHierarchyItem::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1339,6 +1350,160 @@
   }
 }
 
+TEST(SuperTypes, SimpleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]]
+{
+  int a;
+};
+
+struct $Child1Def[[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();
+
+  for (auto Pt : {"p1", "p2", "p3", "p4"}) {
+llvm::Optional Result = getTypeHierarchy(
+AST, Source.point(Pt), 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child2"), WithKind(SymbolKind::Struct),
+Parents(AllOf(
+WithName("Child1"), WithKind(SymbolKind::Struct),
+SelectionRangeIs(Source.range("Child1Def")),
+Parents(AllOf(WithName("Parent"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("ParentDef")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, MultipleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $Parent1Def[[Parent1]]
+{
+  int a;
+};
+
+struct $Parent2Def[[Parent2]]
+{
+  int b;
+};
+
+struct $Parent3Def[[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();
+
+  for (auto Pt : {"c1", "c2", "c3", "c4"}) {
+llvm::Optional Result = getTypeHierarchy(
+AST, Source.point(Pt), 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("Parent1"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent1Def")),
+  Parents()),
+AllOf(WithName("Parent3"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent3Def")),
+  Parents(AllOf(
+  WithName("Parent2"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent2Def")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, OnMethod) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]]
+{
+  void method ();
+  void method () const;
+  void method (int x);
+  void method (char x);
+};
+
+struct $Child1Def[[Child1]] : Parent
+{

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 184985.
nridge added a comment.

remove unrelated file


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -25,9 +25,11 @@
 namespace {
 
 using testing::ElementsAre;
+using testing::Eq;
 using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +41,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(&TypeHierarchyItem::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1339,6 +1350,160 @@
   }
 }
 
+TEST(SuperTypes, SimpleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]]
+{
+  int a;
+};
+
+struct $Child1Def[[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();
+
+  for (auto Pt : {"p1", "p2", "p3", "p4"}) {
+llvm::Optional Result = getTypeHierarchy(
+AST, Source.point(Pt), 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child2"), WithKind(SymbolKind::Struct),
+Parents(AllOf(
+WithName("Child1"), WithKind(SymbolKind::Struct),
+SelectionRangeIs(Source.range("Child1Def")),
+Parents(AllOf(WithName("Parent"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("ParentDef")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, MultipleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $Parent1Def[[Parent1]]
+{
+  int a;
+};
+
+struct $Parent2Def[[Parent2]]
+{
+  int b;
+};
+
+struct $Parent3Def[[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();
+
+  for (auto Pt : {"c1", "c2", "c3", "c4"}) {
+llvm::Optional Result = getTypeHierarchy(
+AST, Source.point(Pt), 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("Parent1"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent1Def")),
+  Parents()),
+AllOf(WithName("Parent3"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent3Def")),
+  Parents(AllOf(
+  WithName("Parent2"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent2Def")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, OnMethod) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]]
+{
+  void method ();
+  void method () const;
+  void method (int x);
+  void method (char x);
+};
+
+struct $Child1Def[[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 = 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 3 inline comments as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/XRefs.cpp:820
 
+// TODO: Reduce duplication between this function and declToSym().
+static llvm::Optional

I am open to feedback on whether we want to reduce the duplication between 
these functions, and if so, suggestions for how.



Comment at: clang-tools-extra/clangd/XRefs.cpp:901
+
+  // TODO: Populate subtypes.
+}

I am deliberately leaving this part for a follow-up patch, as it will require 
index changes.



Comment at: clang-tools-extra/unittests/clangd/Matchers.h:130
 
+// Implements the HasValue(m) matcher for matching an Optional whose
+// value matches matcher m.

Should I split this out into a separate patch? It's used by the tests being 
added for this functionality.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 186136.
nridge added a comment.

Add tests for scenarios involving class template specializations

Also improve support for dependent bases


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -25,9 +25,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +43,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(&TypeHierarchyItem::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1365,6 +1378,291 @@
   }
 }
 
+TEST(SuperTypes, SimpleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]]
+{
+  int a;
+};
+
+struct $Child1Def[[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();
+
+  for (auto Pt : {"p1", "p2", "p3", "p4"}) {
+llvm::Optional Result = getTypeHierarchy(
+AST, Source.point(Pt), 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child2"), WithKind(SymbolKind::Struct),
+Parents(AllOf(
+WithName("Child1"), WithKind(SymbolKind::Struct),
+SelectionRangeIs(Source.range("Child1Def")),
+Parents(AllOf(WithName("Parent"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("ParentDef")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, MultipleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $Parent1Def[[Parent1]]
+{
+  int a;
+};
+
+struct $Parent2Def[[Parent2]]
+{
+  int b;
+};
+
+struct $Parent3Def[[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();
+
+  for (auto Pt : {"c1", "c2", "c3", "c4"}) {
+llvm::Optional Result = getTypeHierarchy(
+AST, Source.point(Pt), 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("Parent1"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent1Def")),
+  Parents()),
+AllOf(WithName("Parent3"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent3Def")),
+  Parents(AllOf(
+  WithName("Parent2"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent2Def")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, OnMethod) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]]
+{
+  void method ();
+  void method () const;
+  void method (int x);
+  void method (char x);
+};
+
+struct $Child1Def[[Child1]] : Parent
+{
+  void method ();
+  void method (char x);
+};
+
+struct Child2 : Child1
+{
+  void me

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D56370#1391904 , @nridge wrote:

> Add tests for scenarios involving class template specializations
>
> Also improve support for dependent bases


(This update is unrelated to the review comments, it's just improvements I had 
in the queue already. Another update to address review comments will come next.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 19 inline comments as done.
nridge added a comment.

Thank you for the reviews!




Comment at: clang-tools-extra/clangd/Protocol.h:1026
+  /// When not defined, it is treated as `0`.
+  llvm::Optional resolve;
+

kadircet wrote:
> why not just use an int then ?
I was making the data structure correspond closely to the protocol, where the 
field is optional. But we can handle this in the serialization logic, yes.



Comment at: clang-tools-extra/clangd/XRefs.cpp:891
+if (Optional ParentSym =
+getTypeHierarchy(*ParentDecl, ASTCtx, Levels - 1, Direction)) {
+  Result->parents->emplace_back(std::move(*ParentSym));

kadircet wrote:
> A problem that might occur when you add children traversal:
> do we really want to include current decl in children of parent when we have 
> `BOTH` as direction? If so, maybe we should rather cache the results? Maybe 
> leave a todo/fixme ?
Actually, this is a bug: if the top-level call uses `Both`, the recursive calls 
should still just use `Parents` and `Children` (i.e. we never want children of 
parents or parents of children). Thanks for spotting that!



Comment at: clang-tools-extra/clangd/XRefs.cpp:925
+CXXRD = VD->getType().getTypePtr()->getAsCXXRecordDecl();
+  } else if (const CXXMethodDecl *Method = dyn_cast(D)) {
+// If this is a method, use the type of the class.

kadircet wrote:
> what about member fields ?
It's not clear what the desired semantics would be for a member field: get the 
type hierarchy of the enclosing class type, or the type hierarchy of the 
field's type?



Comment at: clang-tools-extra/clangd/XRefs.cpp:935
+TypeHierarchyDirection ResolveDirection =
+Direction.getValueOr(TypeHierarchyDirection::Parents);
+return getTypeHierarchy(*CXXRD, ASTCtx, ResolveLevels, ResolveDirection);

kadircet wrote:
> Is this mentioned in proposal?
It's not specified; I left a [comment on the 
proposal](https://github.com/Microsoft/vscode-languageserver-node/pull/426/commits/876d7fe224e17d7c08258a6da21d11a2df9c1d0d#r252942213)
 asking about it.



Comment at: clang-tools-extra/unittests/clangd/Matchers.h:130
 
+// Implements the HasValue(m) matcher for matching an Optional whose
+// value matches matcher m.

sammccall wrote:
> nridge wrote:
> > Should I split this out into a separate patch? It's used by the tests being 
> > added for this functionality.
> This is nice! I think it should probably go in llvm/Testing/Support/..., but 
> it's OK here.
I'll leave it here for now. We can move it in a follow-up patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 186137.
nridge marked 4 inline comments as done.
nridge added a comment.

Address Kadir's review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -25,9 +25,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +43,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(&TypeHierarchyItem::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1365,6 +1378,277 @@
   }
 }
 
+TEST(SuperTypes, SimpleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]] {
+  int a;
+};
+
+struct $Child1Def[[Child1]] : Parent {
+  int b;
+};
+
+struct Ch^ild2 : Child1 {
+  int c;
+};
+
+struct Child3 : Child2 {
+  int d;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+
+  parent.a = 1;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  for (Position Pt : Source.points()) {
+llvm::Optional Result =
+getTypeHierarchy(AST, Pt, 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child2"), WithKind(SymbolKind::Struct),
+Parents(AllOf(
+WithName("Child1"), WithKind(SymbolKind::Struct),
+SelectionRangeIs(Source.range("Child1Def")),
+Parents(AllOf(WithName("Parent"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("ParentDef")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, MultipleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $Parent1Def[[Parent1]] {
+  int a;
+};
+
+struct $Parent2Def[[Parent2]] {
+  int b;
+};
+
+struct $Parent3Def[[Parent3]] : Parent2 {
+  int c;
+};
+
+struct Ch^ild : Parent1, Parent3 {
+  int d;
+};
+
+int main() {
+  Ch^ild  ch$c3^ild;
+
+  ch^ild.a = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  for (Position Pt : Source.points()) {
+llvm::Optional Result =
+getTypeHierarchy(AST, Pt, 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("Parent1"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent1Def")),
+  Parents()),
+AllOf(WithName("Parent3"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent3Def")),
+  Parents(AllOf(
+  WithName("Parent2"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent2Def")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, OnMethod) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]] {
+  void method ();
+  void method () const;
+  void method (int x);
+  void method (char x);
+};
+
+struct $Child1Def[[Child1]] : Parent {
+  void method ();
+  void method (char x);
+};
+
+struct Child2 : Child1 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+struct Child3 : Child2 {
+  void method (int x);
+};
+)cpp");
+
+ 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D56370#1390359 , @sammccall wrote:

> So on a concrete but still high-level:
>
> - I don't think we should implement the `resolve` operation, or resolution 
> bounds, at this point - this patch doesn't do anything slow. Returning 
> complete ancestors and never returning any children seems simplest.


A hypothetical client could always ask for one level at a time, and ignore any 
levels it didn't ask for; such a client would break if we did this.

I think the implementation of `resolve` is straightforward enough that we might 
as well keep it.

> - in 'XRefs.h', I think the API to introduce is `findTypeAt(Position) -> 
> Decl*` +  `typeAncestors(Decl*)` and later `typeDescendants(Decl*)`, rather 
> than a single more complex `typeHierarchy` call. These two operations have 
> little in common implementation-wise, and it's easy to imagine editors 
> preferring to expose them separately. In clangdserver of course we need to 
> expose a single operation because of transactionality. The stitching together 
> could go in clangdserver, or a higher-level function exposed by xrefs - but I 
> think the separate functions are what we should be unit-testing.

This sounds nice and clean, thanks for the suggestion! I will give it a try.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 186139.
nridge added a comment.

Introduce and use findRecordTypeAt() and typeAncestors() helpers


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -25,9 +25,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +43,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(&TypeHierarchyItem::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1365,6 +1378,277 @@
   }
 }
 
+TEST(SuperTypes, SimpleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]] {
+  int a;
+};
+
+struct $Child1Def[[Child1]] : Parent {
+  int b;
+};
+
+struct Ch^ild2 : Child1 {
+  int c;
+};
+
+struct Child3 : Child2 {
+  int d;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+
+  parent.a = 1;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  for (Position Pt : Source.points()) {
+llvm::Optional Result =
+getTypeHierarchy(AST, Pt, 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child2"), WithKind(SymbolKind::Struct),
+Parents(AllOf(
+WithName("Child1"), WithKind(SymbolKind::Struct),
+SelectionRangeIs(Source.range("Child1Def")),
+Parents(AllOf(WithName("Parent"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("ParentDef")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, MultipleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $Parent1Def[[Parent1]] {
+  int a;
+};
+
+struct $Parent2Def[[Parent2]] {
+  int b;
+};
+
+struct $Parent3Def[[Parent3]] : Parent2 {
+  int c;
+};
+
+struct Ch^ild : Parent1, Parent3 {
+  int d;
+};
+
+int main() {
+  Ch^ild  ch$c3^ild;
+
+  ch^ild.a = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  for (Position Pt : Source.points()) {
+llvm::Optional Result =
+getTypeHierarchy(AST, Pt, 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("Parent1"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent1Def")),
+  Parents()),
+AllOf(WithName("Parent3"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent3Def")),
+  Parents(AllOf(
+  WithName("Parent2"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent2Def")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, OnMethod) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]] {
+  void method ();
+  void method () const;
+  void method (int x);
+  void method (char x);
+};
+
+struct $Child1Def[[Child1]] : Parent {
+  void method ();
+  void method (char x);
+};
+
+struct Child2 : Child1 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+struct Child3 : Child2 {
+  void method (int x);
+};
+)cpp");
+
+  TestTU 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D56370#1390359 , @sammccall wrote:

> - in 'XRefs.h', I think the API to introduce is `findTypeAt(Position) -> 
> Decl*` +  `typeAncestors(Decl*)` and later `typeDescendants(Decl*)`, rather 
> than a single more complex `typeHierarchy` call. These two operations have 
> little in common implementation-wise, and it's easy to imagine editors 
> preferring to expose them separately. In clangdserver of course we need to 
> expose a single operation because of transactionality. The stitching together 
> could go in clangdserver, or a higher-level function exposed by xrefs - but I 
> think the separate functions are what we should be unit-testing.


In the latest update I introduced these helpers. I made the first one 
`findRecordTypeAt()` instead of `findTypeAt()`, since `findTypeAt()` suggests 
it also works for built-in types like `int`, but I don't think we can get a 
`Decl*` for `int`.

As far as reworking the tests to use these functions, I've thought about that a 
bit:

- These functions return AST nodes. It's not clear to me how I would come up 
with "expected" AST nodes to test the return values against.
- Even if we find a way to get "expected" AST nodes, we would be losing test 
coverage of functions like `declToTypeHierarchyItem()` (though I suppose we 
could write separate tests for that).
- We could instead test against the //properties// of the AST nodes, such as 
their names and source ranges, but then the test code to query those properties 
would basically be duplicating code in `declToTypeHierarchyItem()`. It would 
seem to make more sense to just test the resulting `TypeHierarchyItem` instead 
(as the tests are doing now).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked an inline comment as done.
nridge added inline comments.
Herald added a subscriber: jdoerfert.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:543
+  return CB(InpAST.takeError());
+CB(clangd::getTypeHierarchy(InpAST->AST, Item.range.start, Resolve,
+Direction));

sammccall wrote:
> relying on the item range to resolve the actual symbol isn't reliable:
>  - the source code may have changed
>  - the range may not be within the TU, and might be e.g. in an indexed header 
> we don't have a compile command for.
Good point. I appreciate a bit better now why you suggested trying to avoid 
`resolve` for now :)

What do you think of the following implementation approach:

 * Use the `data` field of `TypeHierarchyItem` (which the client will send back 
to the server for `resolve` queries) to send the `SymbolID` of the item to the 
client.
 * When the client sends back the `SymbolID` in the `resolve` request, look up 
the symbol in the index, and use the source range from the symbol.

(Later, when we start storing base <--> derived relationships in the index for 
subtype support, we could just answer the entire `resolve` query using the 
index.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 2 inline comments as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:543
+  return CB(InpAST.takeError());
+CB(clangd::getTypeHierarchy(InpAST->AST, Item.range.start, Resolve,
+Direction));

sammccall wrote:
> nridge wrote:
> > sammccall wrote:
> > > relying on the item range to resolve the actual symbol isn't reliable:
> > >  - the source code may have changed
> > >  - the range may not be within the TU, and might be e.g. in an indexed 
> > > header we don't have a compile command for.
> > Good point. I appreciate a bit better now why you suggested trying to avoid 
> > `resolve` for now :)
> > 
> > What do you think of the following implementation approach:
> > 
> >  * Use the `data` field of `TypeHierarchyItem` (which the client will send 
> > back to the server for `resolve` queries) to send the `SymbolID` of the 
> > item to the client.
> >  * When the client sends back the `SymbolID` in the `resolve` request, look 
> > up the symbol in the index, and use the source range from the symbol.
> > 
> > (Later, when we start storing base <--> derived relationships in the index 
> > for subtype support, we could just answer the entire `resolve` query using 
> > the index.)
> Thanks for exploring all the options here!
> 
> Generally we've tried to avoid relying on the index unless it's needed, using 
> the AST where possible. There are failure modes here:
>  - the base type is in the current file, which is actively edited. The ranges 
> in the index may be off due to staleness.
>  - the base type is not indexed, because it is e.g. a member class inside a 
> class template
>  - there's no index (`-index=0`, though I'm not sure why we still support 
> this) or the index is stale and the type is missing (we're working on making 
> index updates more async)
>  - the base type is not encodable.
> 
> There are just more moving pieces here, I think. Is there a clear reason to 
> support resolve for parents?
> Is there a clear reason to support resolve for parents?

Just what I said earlier about a hypothetical client that relies on it.

However, given the complications involved in implementing it, I'm happy with 
only being concerned with actual clients, not hypothetical ones. The only 
client implementation I currently know of is Theia, and I checked that it works 
fine without `resolve`, so I'm happy with deferring work on `resolve` for now.

If another client comes along that relies on `resolve`, we can revisit this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 186944.
nridge added a comment.

- Rework tests to test the lower-level functions like typeAncestors()
- Remove support for typeHierarchy/resolve


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -15,6 +15,8 @@
 #include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -25,9 +27,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +45,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(&TypeHierarchyItem::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1389,6 +1404,337 @@
   }
 }
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+// TODO: FindRecordTypeAt, TemplateSpec
+
+TEST(TypeAncestors, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast(&findDecl(AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast(&findDecl(AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast(&findDecl(AST, "Child2"));
+
+  ASTContext &Ctx = AST.getASTContext();
+
+  EXPECT_THAT(typeAncestors(Ctx, Parent), ElementsAre());
+  EXPECT_THAT(typeAncestors(Ctx, Child1), ElementsAre(Parent));
+  EXPECT_THAT(typeAncestors(Ctx, Child2), ElementsAre(Child1));
+}
+
+TEST(TypeAncestors, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : Parent1, Parent3 {
+  int d;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked an inline comment as done.
nridge added inline comments.



Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1604
+  dyn_cast(&findDecl(AST, 
"Parent"))->getTemplatedDecl();
+  // TODO: This fails because findDecl() doesn't support template-ids
+  // const CXXRecordDecl *ParentSpec =

Any suggestions for how we can make `findDecl()` support //template-id//s?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 186947.
nridge added a comment.

Fix a test failure


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -15,6 +15,8 @@
 #include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -25,9 +27,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +45,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(&TypeHierarchyItem::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1389,6 +1404,337 @@
   }
 }
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(&findDecl(AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+// TODO: FindRecordTypeAt, TemplateSpec
+
+TEST(TypeAncestors, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast(&findDecl(AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast(&findDecl(AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast(&findDecl(AST, "Child2"));
+
+  ASTContext &Ctx = AST.getASTContext();
+
+  EXPECT_THAT(typeAncestors(Ctx, Parent), ElementsAre());
+  EXPECT_THAT(typeAncestors(Ctx, Child1), ElementsAre(Parent));
+  EXPECT_THAT(typeAncestors(Ctx, Child2), ElementsAre(Child1));
+}
+
+TEST(TypeAncestors, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : Parent1, Parent3 {
+  int d;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.get

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 202369.
nridge added a comment.

Address latest review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/Relation.cpp
  clang-tools-extra/clangd/index/Relation.h
  clang-tools-extra/clangd/unittests/IndexTests.cpp

Index: clang-tools-extra/clangd/unittests/IndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/IndexTests.cpp
+++ clang-tools-extra/clangd/unittests/IndexTests.cpp
@@ -76,6 +76,45 @@
 EXPECT_THAT(*S.find(SymbolID(Sym)), Named(Sym));
 }
 
+TEST(RelationSlab, Lookup) {
+  SymbolID A{"A"};
+  SymbolID B{"B"};
+  SymbolID C{"C"};
+  SymbolID D{"D"};
+
+  RelationSlab::Builder Builder;
+  Builder.insert(Relation{A, index::SymbolRole::RelationBaseOf, B});
+  Builder.insert(Relation{A, index::SymbolRole::RelationBaseOf, C});
+  Builder.insert(Relation{B, index::SymbolRole::RelationBaseOf, D});
+  Builder.insert(Relation{C, index::SymbolRole::RelationBaseOf, D});
+  Builder.insert(Relation{B, index::SymbolRole::RelationChildOf, A});
+  Builder.insert(Relation{C, index::SymbolRole::RelationChildOf, A});
+  Builder.insert(Relation{D, index::SymbolRole::RelationChildOf, B});
+  Builder.insert(Relation{D, index::SymbolRole::RelationChildOf, C});
+
+  RelationSlab Slab = std::move(Builder).build();
+  EXPECT_THAT(
+  Slab.lookup(A, index::SymbolRole::RelationBaseOf),
+  UnorderedElementsAre(Relation{A, index::SymbolRole::RelationBaseOf, B},
+   Relation{A, index::SymbolRole::RelationBaseOf, C}));
+}
+
+TEST(RelationSlab, Duplicates) {
+  SymbolID A{"A"};
+  SymbolID B{"B"};
+  SymbolID C{"C"};
+
+  RelationSlab::Builder Builder;
+  Builder.insert(Relation{A, index::SymbolRole::RelationBaseOf, B});
+  Builder.insert(Relation{A, index::SymbolRole::RelationBaseOf, C});
+  Builder.insert(Relation{A, index::SymbolRole::RelationBaseOf, B});
+
+  RelationSlab Slab = std::move(Builder).build();
+  EXPECT_THAT(Slab, UnorderedElementsAre(
+Relation{A, index::SymbolRole::RelationBaseOf, B},
+Relation{A, index::SymbolRole::RelationBaseOf, C}));
+}
+
 TEST(SwapIndexTest, OldIndexRecycled) {
   auto Token = std::make_shared();
   std::weak_ptr WeakToken = Token;
Index: clang-tools-extra/clangd/index/Relation.h
===
--- /dev/null
+++ clang-tools-extra/clangd/index/Relation.h
@@ -0,0 +1,88 @@
+//===--- Relation.h --*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_RELATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_RELATION_H
+
+#include "SymbolID.h"
+#include "SymbolLocation.h"
+#include "clang/Index/IndexSymbol.h"
+#include "llvm/ADT/iterator_range.h"
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+
+/// Represents a relation between two symbols.
+/// For an example "A is a base class of B" may be represented
+/// as { Subject = A, Predicate = RelationBaseOf, Object = B }.
+struct Relation {
+  SymbolID Subject;
+  index::SymbolRole Predicate;
+  SymbolID Object;
+
+  bool operator==(const Relation &Other) const {
+return std::tie(Subject, Predicate, Object) ==
+   std::tie(Other.Subject, Other.Predicate, Other.Object);
+  }
+  // SPO order
+  bool operator<(const Relation &Other) const {
+return std::tie(Subject, Predicate, Object) <
+   std::tie(Other.Subject, Other.Predicate, Other.Object);
+  }
+};
+
+class RelationSlab {
+public:
+  using value_type = Relation;
+  using const_iterator = std::vector::const_iterator;
+  using iterator = const_iterator;
+
+  RelationSlab() = default;
+  RelationSlab(RelationSlab &&Slab) = default;
+  RelationSlab &operator=(RelationSlab &&RHS) = default;
+
+  const_iterator begin() const { return Relations.begin(); }
+  const_iterator end() const { return Relations.end(); }
+  size_t size() const { return Relations.size(); }
+  bool empty() const { return Relations.empty(); }
+
+  size_t bytes() const {
+return sizeof(*this) + sizeof(value_type) * Relations.capacity();
+  }
+
+  /// Lookup all relations matching the given subject and predicate.
+  llvm::iterator_range lookup(const SymbolID &Subject,
+index::SymbolRole Predicate) const;
+
+  /// RelationSlab::Builder is a mutable container that can 'freeze' to
+  /// RelationSlab.
+  class Builder {
+  public:
+/// Adds a relation to the 

[PATCH] D62459: [clangd] Serialization support for RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 202370.
nridge marked 8 inline comments as done.
nridge added a comment.

Addressed most review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62459/new/

https://reviews.llvm.org/D62459

Files:
  clang-tools-extra/clangd/index/Serialization.cpp
  clang-tools-extra/clangd/index/Serialization.h
  clang-tools-extra/clangd/index/YAMLSerialization.cpp
  clang-tools-extra/clangd/unittests/SerializationTests.cpp

Index: clang-tools-extra/clangd/unittests/SerializationTests.cpp
===
--- clang-tools-extra/clangd/unittests/SerializationTests.cpp
+++ clang-tools-extra/clangd/unittests/SerializationTests.cpp
@@ -82,6 +82,14 @@
   End:
 Line: 5
 Column: 8
+...
+--- !Relations
+Subject:
+  ID:  6481EE7AF2841756
+Predicate:   0
+Object:
+  ID:  6512AEC512EA3A2D
+...
 )";
 
 MATCHER_P(ID, I, "") { return arg.ID == cantFail(SymbolID::fromStr(I)); }
@@ -139,6 +147,13 @@
   auto Ref1 = ParsedYAML->Refs->begin()->second.front();
   EXPECT_EQ(Ref1.Kind, RefKind::Reference);
   EXPECT_EQ(StringRef(Ref1.Location.FileURI), "file:///path/foo.cc");
+
+  SymbolID Base = cantFail(SymbolID::fromStr("6481EE7AF2841756"));
+  SymbolID Derived = cantFail(SymbolID::fromStr("6512AEC512EA3A2D"));
+  ASSERT_TRUE(bool(ParsedYAML->Relations));
+  EXPECT_THAT(*ParsedYAML->Relations,
+  UnorderedElementsAre(
+  Relation{Base, index::SymbolRole::RelationBaseOf, Derived}));
 }
 
 std::vector YAMLFromSymbols(const SymbolSlab &Slab) {
@@ -149,8 +164,15 @@
 }
 std::vector YAMLFromRefs(const RefSlab &Slab) {
   std::vector Result;
-  for (const auto &Sym : Slab)
-Result.push_back(toYAML(Sym));
+  for (const auto &Refs : Slab)
+Result.push_back(toYAML(Refs));
+  return Result;
+}
+
+std::vector YAMLFromRelations(const RelationSlab &Slab) {
+  std::vector Result;
+  for (const auto &Rel : Slab)
+Result.push_back(toYAML(Rel));
   return Result;
 }
 
@@ -167,12 +189,15 @@
   ASSERT_TRUE(bool(In2)) << In.takeError();
   ASSERT_TRUE(In2->Symbols);
   ASSERT_TRUE(In2->Refs);
+  ASSERT_TRUE(In2->Relations);
 
   // Assert the YAML serializations match, for nice comparisons and diffs.
   EXPECT_THAT(YAMLFromSymbols(*In2->Symbols),
   UnorderedElementsAreArray(YAMLFromSymbols(*In->Symbols)));
   EXPECT_THAT(YAMLFromRefs(*In2->Refs),
   UnorderedElementsAreArray(YAMLFromRefs(*In->Refs)));
+  EXPECT_THAT(YAMLFromRelations(*In2->Relations),
+  UnorderedElementsAreArray(YAMLFromRelations(*In->Relations)));
 }
 
 TEST(SerializationTest, SrcsTest) {
Index: clang-tools-extra/clangd/index/YAMLSerialization.cpp
===
--- clang-tools-extra/clangd/index/YAMLSerialization.cpp
+++ clang-tools-extra/clangd/index/YAMLSerialization.cpp
@@ -13,6 +13,7 @@
 //===--===//
 
 #include "Index.h"
+#include "Relation.h"
 #include "Serialization.h"
 #include "SymbolLocation.h"
 #include "SymbolOrigin.h"
@@ -35,10 +36,11 @@
 namespace {
 using RefBundle =
 std::pair>;
-// This is a pale imitation of std::variant
+// This is a pale imitation of std::variant
 struct VariantEntry {
   llvm::Optional Symbol;
   llvm::Optional Refs;
+  llvm::Optional Relation;
 };
 // A class helps YAML to serialize the 32-bit encoded position (Line&Column),
 // as YAMLIO can't directly map bitfields.
@@ -53,6 +55,8 @@
 
 using clang::clangd::Ref;
 using clang::clangd::RefKind;
+using clang::clangd::Relation;
+using clang::clangd::RelationKind;
 using clang::clangd::Symbol;
 using clang::clangd::SymbolID;
 using clang::clangd::SymbolLocation;
@@ -60,6 +64,7 @@
 using clang::index::SymbolInfo;
 using clang::index::SymbolKind;
 using clang::index::SymbolLanguage;
+using clang::index::SymbolRole;
 
 // Helper to (de)serialize the SymbolID. We serialize it as a hex string.
 struct NormalizedSymbolID {
@@ -275,6 +280,37 @@
   }
 };
 
+struct NormalizedSymbolRole {
+  NormalizedSymbolRole(IO &) {}
+  NormalizedSymbolRole(IO &IO, SymbolRole R) {
+Kind = static_cast(clang::clangd::symbolRoleToRelationKind(R));
+  }
+
+  SymbolRole denormalize(IO &IO) {
+return clang::clangd::relationKindToSymbolRole(
+static_cast(Kind));
+  }
+
+  uint8_t Kind = 0;
+};
+
+template <> struct MappingTraits {
+  static void mapping(IO &IO, SymbolID &ID) {
+MappingNormalization NSymbolID(IO, ID);
+IO.mapRequired("ID", NSymbolID->HexString);
+  }
+};
+
+template <> struct MappingTraits {
+  static void mapping(IO &IO, Relation &Relation) {
+MappingNormalization NRole(
+IO, Relation.Predicate);
+IO.mapRequired("Subject", Relation.Subject);
+IO.mapRequired("Predicate", NRole->Kind);
+IO.mapRequired("Object", Relation.Object);
+  }
+};
+
 template <> struct MappingTraits {
   static void

[PATCH] D62459: [clangd] Serialization support for RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/index/Serialization.h:81
+// Used for serializing SymbolRole as used in Relation.
+enum class RelationKind : uint8_t { ChildOf = 1, BaseOf };
+llvm::Expected symbolRoleToRelationKind(index::SymbolRole);

nridge wrote:
> kadircet wrote:
> > kadircet wrote:
> > > why not start from zero?
> > > 
> > > Also let's change the `index::SymbolRole` in `Relation` with this one.
> > why not just store baseof ? do we need both directions for typehierarchy?
> > Also let's change the index::SymbolRole in Relation with this one.
> 
> You previously asked for the opposite change in [this 
> comment](https://reviews.llvm.org/D59407?id=190785#inline-525734) :)
> 
> Did your opinion change?
> why not start from zero?

The original motivation was so that we had a distinct value to return in case 
of an error. However, now that we use `llvm_unreachable` that doesn't seem to 
be necessary any more.



Comment at: clang-tools-extra/clangd/index/Serialization.h:81
+// Used for serializing SymbolRole as used in Relation.
+enum class RelationKind : uint8_t { ChildOf = 1, BaseOf };
+llvm::Expected symbolRoleToRelationKind(index::SymbolRole);

nridge wrote:
> nridge wrote:
> > kadircet wrote:
> > > kadircet wrote:
> > > > why not start from zero?
> > > > 
> > > > Also let's change the `index::SymbolRole` in `Relation` with this one.
> > > why not just store baseof ? do we need both directions for typehierarchy?
> > > Also let's change the index::SymbolRole in Relation with this one.
> > 
> > You previously asked for the opposite change in [this 
> > comment](https://reviews.llvm.org/D59407?id=190785#inline-525734) :)
> > 
> > Did your opinion change?
> > why not start from zero?
> 
> The original motivation was so that we had a distinct value to return in case 
> of an error. However, now that we use `llvm_unreachable` that doesn't seem to 
> be necessary any more.
> why not just store baseof ? do we need both directions for typehierarchy?

In the future, we may want to be able to look up supertypes in the index as 
well (for example, to handle scenarios where a type that is only 
forward-declared in the current TU is queried).

However, we can add that later, and just focus on subtypes for now, which only 
require BaseOf.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62459/new/

https://reviews.llvm.org/D62459



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62471: [clangd] SymbolCollector support for relations

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 202371.
nridge marked 11 inline comments as done.
nridge added a comment.

Addressed most review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62471/new/

https://reviews.llvm.org/D62471

Files:
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -123,8 +123,9 @@
 assert(AST.hasValue());
 const NamedDecl &ND =
 Qualified ? findDecl(*AST, Name) : findUnqualifiedDecl(*AST, Name);
-const SourceManager& SM = AST->getSourceManager();
-bool MainFile = SM.isWrittenInMainFile(SM.getExpansionLoc(ND.getBeginLoc()));
+const SourceManager &SM = AST->getSourceManager();
+bool MainFile =
+SM.isWrittenInMainFile(SM.getExpansionLoc(ND.getBeginLoc()));
 return SymbolCollector::shouldCollectSymbol(
 ND, AST->getASTContext(), SymbolCollector::Options(), MainFile);
   }
@@ -272,13 +273,14 @@
 Args, Factory->create(), Files.get(),
 std::make_shared());
 
-InMemoryFileSystem->addFile(
-TestHeaderName, 0, llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+InMemoryFileSystem->addFile(TestHeaderName, 0,
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 InMemoryFileSystem->addFile(TestFileName, 0,
 llvm::MemoryBuffer::getMemBuffer(MainCode));
 Invocation.run();
 Symbols = Factory->Collector->takeSymbols();
 Refs = Factory->Collector->takeRefs();
+Relations = Factory->Collector->takeRelations();
 return true;
   }
 
@@ -290,6 +292,7 @@
   std::string TestFileURI;
   SymbolSlab Symbols;
   RefSlab Refs;
+  RelationSlab Relations;
   SymbolCollector::Options CollectorOpts;
   std::unique_ptr PragmaHandler;
 };
@@ -634,6 +637,19 @@
   HaveRanges(Header.ranges();
 }
 
+TEST_F(SymbolCollectorTest, Relations) {
+  std::string Header = R"(
+  class Base {};
+  class Derived : public Base {};
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  const Symbol &Base = findSymbol(Symbols, "Base");
+  const Symbol &Derived = findSymbol(Symbols, "Derived");
+  EXPECT_THAT(Relations,
+  Contains(Relation{Base.ID, index::SymbolRole::RelationBaseOf,
+Derived.ID}));
+}
+
 TEST_F(SymbolCollectorTest, References) {
   const std::string Header = R"(
 class W;
@@ -783,10 +799,9 @@
 void f1() {}
   )";
   runSymbolCollector(/*Header=*/"", Main);
-  EXPECT_THAT(Symbols,
-  UnorderedElementsAre(QName("Foo"), QName("f1"), QName("f2"),
-   QName("ff"), QName("foo"), QName("foo::Bar"),
-   QName("main_f")));
+  EXPECT_THAT(Symbols, UnorderedElementsAre(
+   QName("Foo"), QName("f1"), QName("f2"), QName("ff"),
+   QName("foo"), QName("foo::Bar"), QName("main_f")));
 }
 
 TEST_F(SymbolCollectorTest, Documentation) {
Index: clang-tools-extra/clangd/index/SymbolCollector.h
===
--- clang-tools-extra/clangd/index/SymbolCollector.h
+++ clang-tools-extra/clangd/index/SymbolCollector.h
@@ -110,6 +110,7 @@
 
   SymbolSlab takeSymbols() { return std::move(Symbols).build(); }
   RefSlab takeRefs() { return std::move(Refs).build(); }
+  RelationSlab takeRelations() { return std::move(Relations).build(); }
 
   void finish() override;
 
@@ -117,6 +118,8 @@
   const Symbol *addDeclaration(const NamedDecl &, SymbolID,
bool IsMainFileSymbol);
   void addDefinition(const NamedDecl &, const Symbol &DeclSymbol);
+  void processRelations(const NamedDecl &ND, const SymbolID &ID,
+ArrayRef Relations);
 
   llvm::Optional getIncludeHeader(llvm::StringRef QName, FileID);
   bool isSelfContainedHeader(FileID);
@@ -135,6 +138,8 @@
   // Only symbols declared in preamble (from #include) and referenced from the
   // main file will be included.
   RefSlab::Builder Refs;
+  // All relations collected from the AST.
+  RelationSlab::Builder Relations;
   ASTContext *ASTCtx;
   std::shared_ptr PP;
   std::shared_ptr CompletionAllocator;
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -193,6 +193,11 @@
   return static_cast(static_cast(RefKind::All) & Roles);
 }
 
+bool shouldIndexRelation(const index::SymbolRelation &R) {
+  // We currently only i

[PATCH] D62471: [clangd] SymbolCollector support for relations

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:298
+
+  processRelations(*ND, *ID, Relations);
+

kadircet wrote:
> why do we want to process these relations for references?
The `RelationBaseOf` is not present in the `Relations` parameter of the 
`handleDeclOccurrence()` call for the base class's declaration. It's only 
present for the specific reference that's in the base-specifier.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:436
+
+if (const auto *CTSD = dyn_cast(Parent)) {
+  if (!CTSD->isExplicitSpecialization()) {

kadircet wrote:
> why? I believe we have those in the index since rL356125.
> 
> even if we don't I believe this part should also be isolated to somehow get 
> canonical version of a given symbol.
I removed this logic as it no longer seems to be necessary now that we use 
libIndex's relations support to compute our relations.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:453
+  this->Relations.insert(
+  Relation{ID, index::SymbolRole::RelationBaseOf, *ParentID});
+}

kadircet wrote:
> kadircet wrote:
> > I believe subject is `ParentID` and object is `ID` in a baseof relation so 
> > these should be replaced ?
> also we should generalize this with something like 
> `index::applyForEachSymbolRole`, which should also get rid of the check at 
> top(`shouldIndexRelation`)
> I believe subject is ParentID and object is ID in a baseof relation so these 
> should be replaced ?

The code was functionally correct, but the name `ParentID` was wrong -- the 
`RelatedSymbol` for a `BaseOf` relation is the child.

Anyways, I'm now calling it `ObjectID`.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:453
+  this->Relations.insert(
+  Relation{ID, index::SymbolRole::RelationBaseOf, *ParentID});
+}

nridge wrote:
> kadircet wrote:
> > kadircet wrote:
> > > I believe subject is `ParentID` and object is `ID` in a baseof relation 
> > > so these should be replaced ?
> > also we should generalize this with something like 
> > `index::applyForEachSymbolRole`, which should also get rid of the check at 
> > top(`shouldIndexRelation`)
> > I believe subject is ParentID and object is ID in a baseof relation so 
> > these should be replaced ?
> 
> The code was functionally correct, but the name `ParentID` was wrong -- the 
> `RelatedSymbol` for a `BaseOf` relation is the child.
> 
> Anyways, I'm now calling it `ObjectID`.
> also we should generalize this with something like 
> index::applyForEachSymbolRole, which should also get rid of the check at 
> top(shouldIndexRelation)

I don't fully understand this suggestion; could you elaborate?

Are you thinking ahead to a future where we want to index additional kinds of 
relations? Can we refactor things at that time?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62471/new/

https://reviews.llvm.org/D62471



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362352: [clangd] Add RelationSlab (authored by nridge, 
committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59407?vs=202369&id=202643#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59407/new/

https://reviews.llvm.org/D59407

Files:
  clang-tools-extra/trunk/clangd/CMakeLists.txt
  clang-tools-extra/trunk/clangd/index/Index.h
  clang-tools-extra/trunk/clangd/index/Relation.cpp
  clang-tools-extra/trunk/clangd/index/Relation.h
  clang-tools-extra/trunk/clangd/unittests/IndexTests.cpp

Index: clang-tools-extra/trunk/clangd/index/Relation.cpp
===
--- clang-tools-extra/trunk/clangd/index/Relation.cpp
+++ clang-tools-extra/trunk/clangd/index/Relation.cpp
@@ -0,0 +1,40 @@
+//===--- Relation.cpp *- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Relation.h"
+
+#include 
+
+namespace clang {
+namespace clangd {
+
+llvm::iterator_range
+RelationSlab::lookup(const SymbolID &Subject,
+ index::SymbolRole Predicate) const {
+  auto IterPair = std::equal_range(Relations.begin(), Relations.end(),
+   Relation{Subject, Predicate, SymbolID{}},
+   [](const Relation &A, const Relation &B) {
+ return std::tie(A.Subject, A.Predicate) <
+std::tie(B.Subject, B.Predicate);
+   });
+  return {IterPair.first, IterPair.second};
+}
+
+RelationSlab RelationSlab::Builder::build() && {
+  // Sort in SPO order.
+  std::sort(Relations.begin(), Relations.end());
+
+  // Remove duplicates.
+  Relations.erase(std::unique(Relations.begin(), Relations.end()),
+  Relations.end());
+
+  return RelationSlab{std::move(Relations)};
+}
+
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/trunk/clangd/index/Relation.h
===
--- clang-tools-extra/trunk/clangd/index/Relation.h
+++ clang-tools-extra/trunk/clangd/index/Relation.h
@@ -0,0 +1,88 @@
+//===--- Relation.h --*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_RELATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_RELATION_H
+
+#include "SymbolID.h"
+#include "SymbolLocation.h"
+#include "clang/Index/IndexSymbol.h"
+#include "llvm/ADT/iterator_range.h"
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+
+/// Represents a relation between two symbols.
+/// For an example "A is a base class of B" may be represented
+/// as { Subject = A, Predicate = RelationBaseOf, Object = B }.
+struct Relation {
+  SymbolID Subject;
+  index::SymbolRole Predicate;
+  SymbolID Object;
+
+  bool operator==(const Relation &Other) const {
+return std::tie(Subject, Predicate, Object) ==
+   std::tie(Other.Subject, Other.Predicate, Other.Object);
+  }
+  // SPO order
+  bool operator<(const Relation &Other) const {
+return std::tie(Subject, Predicate, Object) <
+   std::tie(Other.Subject, Other.Predicate, Other.Object);
+  }
+};
+
+class RelationSlab {
+public:
+  using value_type = Relation;
+  using const_iterator = std::vector::const_iterator;
+  using iterator = const_iterator;
+
+  RelationSlab() = default;
+  RelationSlab(RelationSlab &&Slab) = default;
+  RelationSlab &operator=(RelationSlab &&RHS) = default;
+
+  const_iterator begin() const { return Relations.begin(); }
+  const_iterator end() const { return Relations.end(); }
+  size_t size() const { return Relations.size(); }
+  bool empty() const { return Relations.empty(); }
+
+  size_t bytes() const {
+return sizeof(*this) + sizeof(value_type) * Relations.capacity();
+  }
+
+  /// Lookup all relations matching the given subject and predicate.
+  llvm::iterator_range lookup(const SymbolID &Subject,
+index::SymbolRole Predicate) const;
+
+  /// RelationSlab::Builder is a mutable container that can 'freeze' to
+  /// RelationSlab.
+  class Builder {
+  public:
+/// Adds a relation to the slab.
+void insert(const Relation &R) { Relations.push_back(R); }
+

[PATCH] D62459: [clangd] Serialization support for RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
nridge marked 4 inline comments as done.
Closed by commit rL362353: [clangd] Serialization support for RelationSlab 
(authored by nridge, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D62459?vs=202370&id=202645#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62459/new/

https://reviews.llvm.org/D62459

Files:
  clang-tools-extra/trunk/clangd/index/Serialization.cpp
  clang-tools-extra/trunk/clangd/index/Serialization.h
  clang-tools-extra/trunk/clangd/index/YAMLSerialization.cpp
  clang-tools-extra/trunk/clangd/unittests/SerializationTests.cpp

Index: clang-tools-extra/trunk/clangd/index/Serialization.cpp
===
--- clang-tools-extra/trunk/clangd/index/Serialization.cpp
+++ clang-tools-extra/trunk/clangd/index/Serialization.cpp
@@ -24,6 +24,29 @@
   return llvm::make_error(Msg,
  llvm::inconvertibleErrorCode());
 }
+} // namespace
+
+RelationKind symbolRoleToRelationKind(index::SymbolRole Role) {
+  // SymbolRole is used to record relations in the index.
+  // Only handle the relations we actually store currently.
+  // If we start storing more relations, this list can be expanded.
+  switch (Role) {
+  case index::SymbolRole::RelationBaseOf:
+return RelationKind::BaseOf;
+  default:
+llvm_unreachable("Unsupported symbol role");
+  }
+}
+
+index::SymbolRole relationKindToSymbolRole(RelationKind Kind) {
+  switch (Kind) {
+  case RelationKind::BaseOf:
+return index::SymbolRole::RelationBaseOf;
+  }
+  llvm_unreachable("Invalid relation kind");
+}
+
+namespace {
 
 // IO PRIMITIVES
 // We use little-endian 32 bit ints, sometimes with variable-length encoding.
@@ -358,6 +381,28 @@
   return Result;
 }
 
+// RELATIONS ENCODING
+// A relations section is a flat list of relations. Each relation has:
+//  - SymbolID (subject): 8 bytes
+//  - relation kind (predicate): 1 byte
+//  - SymbolID (object): 8 bytes
+// In the future, we might prefer a packed representation if the need arises.
+
+void writeRelation(const Relation &R, llvm::raw_ostream &OS) {
+  OS << R.Subject.raw();
+  RelationKind Kind = symbolRoleToRelationKind(R.Predicate);
+  OS.write(static_cast(Kind));
+  OS << R.Object.raw();
+}
+
+Relation readRelation(Reader &Data) {
+  SymbolID Subject = Data.consumeID();
+  index::SymbolRole Predicate =
+  relationKindToSymbolRole(static_cast(Data.consume8()));
+  SymbolID Object = Data.consumeID();
+  return {Subject, Predicate, Object};
+}
+
 // FILE ENCODING
 // A file is a RIFF chunk with type 'CdIx'.
 // It contains the sections:
@@ -434,6 +479,17 @@
   return makeError("malformed or truncated refs");
 Result.Refs = std::move(Refs).build();
   }
+  if (Chunks.count("rela")) {
+Reader RelationsReader(Chunks.lookup("rela"));
+RelationSlab::Builder Relations;
+while (!RelationsReader.eof()) {
+  auto Relation = readRelation(RelationsReader);
+  Relations.insert(Relation);
+}
+if (RelationsReader.err())
+  return makeError("malformed or truncated relations");
+Result.Relations = std::move(Relations).build();
+  }
   return std::move(Result);
 }
 
@@ -483,6 +539,14 @@
 }
   }
 
+  std::vector Relations;
+  if (Data.Relations) {
+for (const auto &Relation : *Data.Relations) {
+  Relations.emplace_back(Relation);
+  // No strings to be interned in relations.
+}
+  }
+
   std::string StringSection;
   {
 llvm::raw_string_ostream StringOS(StringSection);
@@ -508,6 +572,16 @@
 RIFF.Chunks.push_back({riff::fourCC("refs"), RefsSection});
   }
 
+  std::string RelationSection;
+  if (Data.Relations) {
+{
+  llvm::raw_string_ostream RelationOS{RelationSection};
+  for (const auto &Relation : Relations)
+writeRelation(Relation, RelationOS);
+}
+RIFF.Chunks.push_back({riff::fourCC("rela"), RelationSection});
+  }
+
   std::string SrcsSection;
   {
 {
@@ -561,6 +635,7 @@
 
   SymbolSlab Symbols;
   RefSlab Refs;
+  RelationSlab Relations;
   {
 trace::Span Tracer("ParseIndex");
 if (auto I = readIndexFile(Buffer->get()->getBuffer())) {
@@ -568,6 +643,8 @@
 Symbols = std::move(*I->Symbols);
   if (I->Refs)
 Refs = std::move(*I->Refs);
+  if (I->Relations)
+Relations = std::move(*I->Relations);
 } else {
   llvm::errs() << "Bad Index: " << llvm::toString(I.takeError()) << "\n";
   return nullptr;
@@ -576,15 +653,17 @@
 
   size_t NumSym = Symbols.size();
   size_t NumRefs = Refs.numRefs();
+  size_t NumRelations = Relations.size();
 
   trace::Span Tracer("BuildIndex");
   auto Index = UseDex ? dex::Dex::build(std::move(Symbols), std::move(Refs))
   : MemIndex::build(std::move(Symbols), std::move(Refs));
   vlog("Loaded {0} from {1} 

[PATCH] D62471: [clangd] SymbolCollector support for relations

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 202646.
nridge marked 5 inline comments as done.
nridge added a comment.

Address remaining review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62471/new/

https://reviews.llvm.org/D62471

Files:
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -123,8 +123,9 @@
 assert(AST.hasValue());
 const NamedDecl &ND =
 Qualified ? findDecl(*AST, Name) : findUnqualifiedDecl(*AST, Name);
-const SourceManager& SM = AST->getSourceManager();
-bool MainFile = SM.isWrittenInMainFile(SM.getExpansionLoc(ND.getBeginLoc()));
+const SourceManager &SM = AST->getSourceManager();
+bool MainFile =
+SM.isWrittenInMainFile(SM.getExpansionLoc(ND.getBeginLoc()));
 return SymbolCollector::shouldCollectSymbol(
 ND, AST->getASTContext(), SymbolCollector::Options(), MainFile);
   }
@@ -272,13 +273,14 @@
 Args, Factory->create(), Files.get(),
 std::make_shared());
 
-InMemoryFileSystem->addFile(
-TestHeaderName, 0, llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+InMemoryFileSystem->addFile(TestHeaderName, 0,
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 InMemoryFileSystem->addFile(TestFileName, 0,
 llvm::MemoryBuffer::getMemBuffer(MainCode));
 Invocation.run();
 Symbols = Factory->Collector->takeSymbols();
 Refs = Factory->Collector->takeRefs();
+Relations = Factory->Collector->takeRelations();
 return true;
   }
 
@@ -290,6 +292,7 @@
   std::string TestFileURI;
   SymbolSlab Symbols;
   RefSlab Refs;
+  RelationSlab Relations;
   SymbolCollector::Options CollectorOpts;
   std::unique_ptr PragmaHandler;
 };
@@ -634,6 +637,19 @@
   HaveRanges(Header.ranges();
 }
 
+TEST_F(SymbolCollectorTest, Relations) {
+  std::string Header = R"(
+  class Base {};
+  class Derived : public Base {};
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  const Symbol &Base = findSymbol(Symbols, "Base");
+  const Symbol &Derived = findSymbol(Symbols, "Derived");
+  EXPECT_THAT(Relations,
+  Contains(Relation{Base.ID, index::SymbolRole::RelationBaseOf,
+Derived.ID}));
+}
+
 TEST_F(SymbolCollectorTest, References) {
   const std::string Header = R"(
 class W;
@@ -783,10 +799,9 @@
 void f1() {}
   )";
   runSymbolCollector(/*Header=*/"", Main);
-  EXPECT_THAT(Symbols,
-  UnorderedElementsAre(QName("Foo"), QName("f1"), QName("f2"),
-   QName("ff"), QName("foo"), QName("foo::Bar"),
-   QName("main_f")));
+  EXPECT_THAT(Symbols, UnorderedElementsAre(
+   QName("Foo"), QName("f1"), QName("f2"), QName("ff"),
+   QName("foo"), QName("foo::Bar"), QName("main_f")));
 }
 
 TEST_F(SymbolCollectorTest, Documentation) {
Index: clang-tools-extra/clangd/index/SymbolCollector.h
===
--- clang-tools-extra/clangd/index/SymbolCollector.h
+++ clang-tools-extra/clangd/index/SymbolCollector.h
@@ -110,6 +110,7 @@
 
   SymbolSlab takeSymbols() { return std::move(Symbols).build(); }
   RefSlab takeRefs() { return std::move(Refs).build(); }
+  RelationSlab takeRelations() { return std::move(Relations).build(); }
 
   void finish() override;
 
@@ -117,6 +118,8 @@
   const Symbol *addDeclaration(const NamedDecl &, SymbolID,
bool IsMainFileSymbol);
   void addDefinition(const NamedDecl &, const Symbol &DeclSymbol);
+  void processRelations(const NamedDecl &ND, const SymbolID &ID,
+ArrayRef Relations);
 
   llvm::Optional getIncludeHeader(llvm::StringRef QName, FileID);
   bool isSelfContainedHeader(FileID);
@@ -135,6 +138,8 @@
   // Only symbols declared in preamble (from #include) and referenced from the
   // main file will be included.
   RefSlab::Builder Refs;
+  // All relations collected from the AST.
+  RelationSlab::Builder Relations;
   ASTContext *ASTCtx;
   std::shared_ptr PP;
   std::shared_ptr CompletionAllocator;
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -193,6 +193,11 @@
   return static_cast(static_cast(RefKind::All) & Roles);
 }
 
+bool shouldIndexRelation(const index::SymbolRelation &R) {
+  // We currently only

[PATCH] D62471: [clangd] SymbolCollector support for relations

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D62471#1527085 , @kadircet wrote:

> LGTM, thanks!


Thanks for the reviews so far!

> I suppose there are only two patches left now?
> 
> - Exposing relations in `SymbolIndex`.
> - Implementing children resolution using that information.

Yup. Coming up in the near future :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62471/new/

https://reviews.llvm.org/D62471



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62471: [clangd] SymbolCollector support for relations

[Nathan Ridge via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362467: [clangd] SymbolCollector support for relations 
(authored by nridge, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D62471?vs=202646&id=202849#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62471/new/

https://reviews.llvm.org/D62471

Files:
  clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
  clang-tools-extra/trunk/clangd/index/SymbolCollector.h
  clang-tools-extra/trunk/clangd/unittests/SymbolCollectorTests.cpp

Index: clang-tools-extra/trunk/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SymbolCollectorTests.cpp
@@ -123,8 +123,9 @@
 assert(AST.hasValue());
 const NamedDecl &ND =
 Qualified ? findDecl(*AST, Name) : findUnqualifiedDecl(*AST, Name);
-const SourceManager& SM = AST->getSourceManager();
-bool MainFile = SM.isWrittenInMainFile(SM.getExpansionLoc(ND.getBeginLoc()));
+const SourceManager &SM = AST->getSourceManager();
+bool MainFile =
+SM.isWrittenInMainFile(SM.getExpansionLoc(ND.getBeginLoc()));
 return SymbolCollector::shouldCollectSymbol(
 ND, AST->getASTContext(), SymbolCollector::Options(), MainFile);
   }
@@ -272,13 +273,14 @@
 Args, Factory->create(), Files.get(),
 std::make_shared());
 
-InMemoryFileSystem->addFile(
-TestHeaderName, 0, llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+InMemoryFileSystem->addFile(TestHeaderName, 0,
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 InMemoryFileSystem->addFile(TestFileName, 0,
 llvm::MemoryBuffer::getMemBuffer(MainCode));
 Invocation.run();
 Symbols = Factory->Collector->takeSymbols();
 Refs = Factory->Collector->takeRefs();
+Relations = Factory->Collector->takeRelations();
 return true;
   }
 
@@ -290,6 +292,7 @@
   std::string TestFileURI;
   SymbolSlab Symbols;
   RefSlab Refs;
+  RelationSlab Relations;
   SymbolCollector::Options CollectorOpts;
   std::unique_ptr PragmaHandler;
 };
@@ -634,6 +637,19 @@
   HaveRanges(Header.ranges();
 }
 
+TEST_F(SymbolCollectorTest, Relations) {
+  std::string Header = R"(
+  class Base {};
+  class Derived : public Base {};
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  const Symbol &Base = findSymbol(Symbols, "Base");
+  const Symbol &Derived = findSymbol(Symbols, "Derived");
+  EXPECT_THAT(Relations,
+  Contains(Relation{Base.ID, index::SymbolRole::RelationBaseOf,
+Derived.ID}));
+}
+
 TEST_F(SymbolCollectorTest, References) {
   const std::string Header = R"(
 class W;
@@ -783,10 +799,9 @@
 void f1() {}
   )";
   runSymbolCollector(/*Header=*/"", Main);
-  EXPECT_THAT(Symbols,
-  UnorderedElementsAre(QName("Foo"), QName("f1"), QName("f2"),
-   QName("ff"), QName("foo"), QName("foo::Bar"),
-   QName("main_f")));
+  EXPECT_THAT(Symbols, UnorderedElementsAre(
+   QName("Foo"), QName("f1"), QName("f2"), QName("ff"),
+   QName("foo"), QName("foo::Bar"), QName("main_f")));
 }
 
 TEST_F(SymbolCollectorTest, Documentation) {
Index: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
@@ -193,6 +193,11 @@
   return static_cast(static_cast(RefKind::All) & Roles);
 }
 
+bool shouldIndexRelation(const index::SymbolRelation &R) {
+  // We currently only index BaseOf relations, for type hierarchy subtypes.
+  return R.Roles & static_cast(index::SymbolRole::RelationBaseOf);
+}
+
 } // namespace
 
 SymbolCollector::SymbolCollector(Options Opts) : Opts(std::move(Opts)) {}
@@ -291,6 +296,16 @@
   SM.getFileID(SpellingLoc) == SM.getMainFileID())
 ReferencedDecls.insert(ND);
 
+  auto ID = getSymbolID(ND);
+  if (!ID)
+return true;
+
+  // Note: we need to process relations for all decl occurrences, including
+  // refs, because the indexing code only populates relations for specific
+  // occurrences. For example, RelationBaseOf is only populated for the
+  // occurrence inside the base-specifier.
+  processRelations(*ND, *ID, Relations);
+
   bool CollectRef = static_cast(Opts.RefFilter) & Roles;
   bool IsOnlyRef =
   !(Roles & (static_cast(index::SymbolRole::Declaration) |
@@ -315,10 +330,6 @@
   if (IsOnlyRef)
 return true;
 
-  auto ID = getSymbolID(ND);
-  if (!ID)
-return true;
-
   // FIXME: ObjCPropertyDec

[PATCH] D62839: [WIP] [clangd] Index API and implementations for relations

[Nathan Ridge via Phabricator via cfe-commits]

nridge created this revision.
nridge added a reviewer: kadircet.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

Remaining work:

- BackgroundIndex implementation
- Dex implementation


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D62839

Files:
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/index/FileIndex.cpp
  clang-tools-extra/clangd/index/FileIndex.h
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/IndexAction.cpp
  clang-tools-extra/clangd/index/IndexAction.h
  clang-tools-extra/clangd/index/MemIndex.cpp
  clang-tools-extra/clangd/index/MemIndex.h
  clang-tools-extra/clangd/index/Merge.cpp
  clang-tools-extra/clangd/index/Merge.h
  clang-tools-extra/clangd/index/Serialization.cpp
  clang-tools-extra/clangd/index/dex/Dex.cpp
  clang-tools-extra/clangd/index/dex/Dex.h
  clang-tools-extra/clangd/indexer/IndexerMain.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/IndexActionTests.cpp
  clang-tools-extra/clangd/unittests/IndexTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -75,7 +75,9 @@
 
 std::unique_ptr TestTU::index() const {
   auto AST = build();
-  auto Idx = llvm::make_unique(/*UseDex=*/true);
+  // UseDex is temporarily set to false so we can test subtypes support
+  // before implementing it in Dex.
+  auto Idx = llvm::make_unique(/*UseDex=*/false);
   Idx->updatePreamble(Filename, AST.getASTContext(), AST.getPreprocessorPtr(),
   AST.getCanonicalIncludes());
   Idx->updateMain(Filename, AST);
Index: clang-tools-extra/clangd/unittests/IndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/IndexTests.cpp
+++ clang-tools-extra/clangd/unittests/IndexTests.cpp
@@ -119,8 +119,9 @@
   auto Token = std::make_shared();
   std::weak_ptr WeakToken = Token;
 
-  SwapIndex S(llvm::make_unique(
-  SymbolSlab(), RefSlab(), std::move(Token), /*BackingDataSize=*/0));
+  SwapIndex S(llvm::make_unique(SymbolSlab(), RefSlab(),
+  RelationSlab(), std::move(Token),
+  /*BackingDataSize=*/0));
   EXPECT_FALSE(WeakToken.expired());  // Current MemIndex keeps it alive.
   S.reset(llvm::make_unique()); // Now the MemIndex is destroyed.
   EXPECT_TRUE(WeakToken.expired());   // So the token is too.
@@ -132,12 +133,13 @@
   FuzzyFindRequest Req;
   Req.Query = "2";
   Req.AnyScope = true;
-  MemIndex I(Symbols, RefSlab());
+  MemIndex I(Symbols, RefSlab(), RelationSlab());
   EXPECT_THAT(match(I, Req), ElementsAre("2"));
 }
 
 TEST(MemIndexTest, MemIndexLimitedNumMatches) {
-  auto I = MemIndex::build(generateNumSymbols(0, 100), RefSlab());
+  auto I =
+  MemIndex::build(generateNumSymbols(0, 100), RefSlab(), RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "5";
   Req.AnyScope = true;
@@ -152,7 +154,7 @@
 TEST(MemIndexTest, FuzzyMatch) {
   auto I = MemIndex::build(
   generateSymbols({"LaughingOutLoud", "LionPopulation", "LittleOldLady"}),
-  RefSlab());
+  RefSlab(), RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "lol";
   Req.AnyScope = true;
@@ -162,8 +164,8 @@
 }
 
 TEST(MemIndexTest, MatchQualifiedNamesWithoutSpecificScope) {
-  auto I =
-  MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab());
+  auto I = MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab(),
+   RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.AnyScope = true;
@@ -171,8 +173,8 @@
 }
 
 TEST(MemIndexTest, MatchQualifiedNamesWithGlobalScope) {
-  auto I =
-  MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab());
+  auto I = MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab(),
+   RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.Scopes = {""};
@@ -181,7 +183,8 @@
 
 TEST(MemIndexTest, MatchQualifiedNamesWithOneScope) {
   auto I = MemIndex::build(
-  generateSymbols({"a::y1", "a::y2", "a::x", "b::y2", "y3"}), RefSlab());
+  generateSymbols({"a::y1", "a::y2", "a::x", "b::y2", "y3"}), RefSlab(),
+  RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.Scopes = {"a::"};
@@ -190,7 +193,8 @@
 
 TEST(MemIndexTest, MatchQualifiedNamesWithMultipleScopes) {
   auto I = MemIndex::build(
-  generateSymbols({"a::y1", "a::y2", "a::x", "b::y3", "y3"}), RefSlab());
+  generateSymbols({"a::y1"

[PATCH] D62839: [WIP] [clangd] Index API and implementations for relations

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

This isn't quite ready for a thorough review, but I have a question for now: in 
`BackgroundIndex::update`, there is a step where we partition symbols and 
references into files.

For relations, should we include a copy in both the file containing the 
definition of the subject, and (if different) the file containing the 
definition of the object?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62839/new/

https://reviews.llvm.org/D62839



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62839: [WIP] [clangd] Index API and implementations for relations

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 203072.
nridge added a comment.

Fill in BackgroundIndex and Dex implementations


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62839/new/

https://reviews.llvm.org/D62839

Files:
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/index/FileIndex.cpp
  clang-tools-extra/clangd/index/FileIndex.h
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/IndexAction.cpp
  clang-tools-extra/clangd/index/IndexAction.h
  clang-tools-extra/clangd/index/MemIndex.cpp
  clang-tools-extra/clangd/index/MemIndex.h
  clang-tools-extra/clangd/index/Merge.cpp
  clang-tools-extra/clangd/index/Merge.h
  clang-tools-extra/clangd/index/Serialization.cpp
  clang-tools-extra/clangd/index/dex/Dex.cpp
  clang-tools-extra/clangd/index/dex/Dex.h
  clang-tools-extra/clangd/indexer/IndexerMain.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/DexTests.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/IndexActionTests.cpp
  clang-tools-extra/clangd/unittests/IndexTests.cpp

Index: clang-tools-extra/clangd/unittests/IndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/IndexTests.cpp
+++ clang-tools-extra/clangd/unittests/IndexTests.cpp
@@ -119,8 +119,9 @@
   auto Token = std::make_shared();
   std::weak_ptr WeakToken = Token;
 
-  SwapIndex S(llvm::make_unique(
-  SymbolSlab(), RefSlab(), std::move(Token), /*BackingDataSize=*/0));
+  SwapIndex S(llvm::make_unique(SymbolSlab(), RefSlab(),
+  RelationSlab(), std::move(Token),
+  /*BackingDataSize=*/0));
   EXPECT_FALSE(WeakToken.expired());  // Current MemIndex keeps it alive.
   S.reset(llvm::make_unique()); // Now the MemIndex is destroyed.
   EXPECT_TRUE(WeakToken.expired());   // So the token is too.
@@ -132,12 +133,13 @@
   FuzzyFindRequest Req;
   Req.Query = "2";
   Req.AnyScope = true;
-  MemIndex I(Symbols, RefSlab());
+  MemIndex I(Symbols, RefSlab(), RelationSlab());
   EXPECT_THAT(match(I, Req), ElementsAre("2"));
 }
 
 TEST(MemIndexTest, MemIndexLimitedNumMatches) {
-  auto I = MemIndex::build(generateNumSymbols(0, 100), RefSlab());
+  auto I =
+  MemIndex::build(generateNumSymbols(0, 100), RefSlab(), RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "5";
   Req.AnyScope = true;
@@ -152,7 +154,7 @@
 TEST(MemIndexTest, FuzzyMatch) {
   auto I = MemIndex::build(
   generateSymbols({"LaughingOutLoud", "LionPopulation", "LittleOldLady"}),
-  RefSlab());
+  RefSlab(), RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "lol";
   Req.AnyScope = true;
@@ -162,8 +164,8 @@
 }
 
 TEST(MemIndexTest, MatchQualifiedNamesWithoutSpecificScope) {
-  auto I =
-  MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab());
+  auto I = MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab(),
+   RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.AnyScope = true;
@@ -171,8 +173,8 @@
 }
 
 TEST(MemIndexTest, MatchQualifiedNamesWithGlobalScope) {
-  auto I =
-  MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab());
+  auto I = MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab(),
+   RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.Scopes = {""};
@@ -181,7 +183,8 @@
 
 TEST(MemIndexTest, MatchQualifiedNamesWithOneScope) {
   auto I = MemIndex::build(
-  generateSymbols({"a::y1", "a::y2", "a::x", "b::y2", "y3"}), RefSlab());
+  generateSymbols({"a::y1", "a::y2", "a::x", "b::y2", "y3"}), RefSlab(),
+  RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.Scopes = {"a::"};
@@ -190,7 +193,8 @@
 
 TEST(MemIndexTest, MatchQualifiedNamesWithMultipleScopes) {
   auto I = MemIndex::build(
-  generateSymbols({"a::y1", "a::y2", "a::x", "b::y3", "y3"}), RefSlab());
+  generateSymbols({"a::y1", "a::y2", "a::x", "b::y3", "y3"}), RefSlab(),
+  RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.Scopes = {"a::", "b::"};
@@ -198,7 +202,8 @@
 }
 
 TEST(MemIndexTest, NoMatchNestedScopes) {
-  auto I = MemIndex::build(generateSymbols({"a::y1", "a::b::y2"}), RefSlab());
+  auto I = MemIndex::build(generateSymbols({"a::y1", "a::b::y2"}), RefSlab(),
+   RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.Scopes = {"a::"};
@@ -206,7 +211,8 @@
 }
 
 TEST(MemIndexTest, IgnoreCases) {
-  auto I = MemIndex::build(generateSymbols({"ns::ABC", "ns::abc"}), RefSlab());
+  auto I = MemIndex::build(generateSymbols({"ns::ABC", "ns::abc"}), RefSlab(),
+   RelationS

[PATCH] D62839: [clangd] Index API and implementations for relations

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Ok, this is probably ready for a first round of review now.

I know I should add new test cases to at least DexTests and 
BackgroundIndexTests, I'll do this in the next version of the patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62839/new/

https://reviews.llvm.org/D62839



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58880: [WIP] [clangd] Type hierarchy subtypes

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 203073.
nridge added a comment.

Rebased onto D62839  and predecessors


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58880/new/

https://reviews.llvm.org/D58880

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp

Index: clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
===
--- clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
+++ clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
@@ -450,6 +450,170 @@
   SelectionRangeIs(Source.range("SDef")), Parents();
 }
 
+SymbolID findSymbolIDByName(llvm::StringRef Name, SymbolIndex *Index) {
+  SymbolID Result;
+  FuzzyFindRequest Request;
+  Request.Query = Name;
+  Request.AnyScope = true;
+  Request.Limit = 1;
+  int ResultCount = 0;
+  Index->fuzzyFind(Request, [&](const Symbol &S) {
+Result = S.ID;
+++ResultCount;
+  });
+  EXPECT_EQ(1, ResultCount);
+  return Result;
+}
+
+std::vector collectSubtypes(SymbolID Type, SymbolIndex *Index) {
+  std::vector Result;
+  Index->relations(Type, index::SymbolRole::RelationBaseOf,
+   [&Result](const SymbolID &ID) { Result.push_back(ID); });
+  return Result;
+}
+
+TEST(Subtypes, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto Index = TU.index();
+
+  SymbolID Parent = findSymbolIDByName("Parent", Index.get());
+  SymbolID Child1 = findSymbolIDByName("Child1", Index.get());
+  SymbolID Child2 = findSymbolIDByName("Child2", Index.get());
+
+  EXPECT_THAT(collectSubtypes(Parent, Index.get()), ElementsAre(Child1));
+  EXPECT_THAT(collectSubtypes(Child1, Index.get()), ElementsAre(Child2));
+}
+
+TEST(Subtypes, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : Parent1, Parent3 {
+  int d;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto Index = TU.index();
+
+  SymbolID Parent1 = findSymbolIDByName("Parent1", Index.get());
+  SymbolID Parent2 = findSymbolIDByName("Parent2", Index.get());
+  SymbolID Parent3 = findSymbolIDByName("Parent3", Index.get());
+  SymbolID Child = findSymbolIDByName("Child", Index.get());
+
+  EXPECT_THAT(collectSubtypes(Parent1, Index.get()), ElementsAre(Child));
+  EXPECT_THAT(collectSubtypes(Parent2, Index.get()), ElementsAre(Parent3));
+  EXPECT_THAT(collectSubtypes(Parent3, Index.get()), ElementsAre(Child));
+}
+
+TEST(Subtypes, ClassTemplate) {
+  Annotations Source(R"cpp(
+struct Parent {};
+
+template 
+struct Child : Parent {};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto Index = TU.index();
+
+  SymbolID Parent = findSymbolIDByName("Parent", Index.get());
+  SymbolID Child = findSymbolIDByName("Child", Index.get());
+
+  EXPECT_THAT(collectSubtypes(Parent, Index.get()), ElementsAre(Child));
+}
+
+// FIXME(nridge): I was expecting https://reviews.llvm.org/D59354 to make this
+// pass.
+TEST(Subtypes, DISABLED_TemplateSpec1) {
+  Annotations Source(R"cpp(
+template 
+struct Parent {};
+
+template <>
+struct Parent {};
+
+struct Child1 : Parent {};
+
+struct Child2 : Parent {};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto Index = TU.index();
+
+  SymbolID Parent = findSymbolIDByName("Parent", Index.get());
+  SymbolID ParentSpec = findSymbolIDByName("Parent", Index.get());
+  SymbolID Child1 = findSymbolIDByName("Child1", Index.get());
+  SymbolID Child2 = findSymbolIDByName("Child2", Index.get());
+
+  EXPECT_THAT(collectSubtypes(Parent, Index.get()), ElementsAre(Child1));
+  EXPECT_THAT(collectSubtypes(ParentSpec, Index.get()), ElementsAre(Child2));
+}
+
+// FIXME(nridge): I was expecting https://reviews.llvm.org/D59354 to make this
+// pass.
+TEST(Subtypes, DISABLED_TemplateSpec2) {
+  Annotations Source(R"cpp(
+struct Parent {};
+
+template 
+struct Child {};
+
+template <>
+struct Child : Parent {};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto Index = TU.index();
+
+  SymbolID Parent = findSymbolIDByName("Parent", Index.get());
+  SymbolID ChildSpec = findSymbolIDByName("Child", Index.get());
+
+  EXPECT_THAT(collectSubtypes(Parent, Index.get()), ElementsAre(ChildSpec));
+}
+
+TEST(Subtypes, DependentBase) {
+  Annotations Source(R"cpp(
+template 
+struct Parent {};
+
+template 
+struct Child : Parent {};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto Index = TU.index();
+
+  SymbolID Parent = findSymbolIDByName("Parent", Index.get());
+  SymbolID Child = findSymbolIDByName("Child", Index.get()

[PATCH] D62839: [clangd] Index API and implementations for relations

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 203495.
nridge marked 16 inline comments as done.
nridge added a comment.

Addressed most review comments, also added some tests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62839/new/

https://reviews.llvm.org/D62839

Files:
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/index/FileIndex.cpp
  clang-tools-extra/clangd/index/FileIndex.h
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/IndexAction.cpp
  clang-tools-extra/clangd/index/IndexAction.h
  clang-tools-extra/clangd/index/MemIndex.cpp
  clang-tools-extra/clangd/index/MemIndex.h
  clang-tools-extra/clangd/index/Merge.cpp
  clang-tools-extra/clangd/index/Merge.h
  clang-tools-extra/clangd/index/Relation.h
  clang-tools-extra/clangd/index/Serialization.cpp
  clang-tools-extra/clangd/index/dex/Dex.cpp
  clang-tools-extra/clangd/index/dex/Dex.h
  clang-tools-extra/clangd/indexer/IndexerMain.cpp
  clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/DexTests.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/IndexActionTests.cpp
  clang-tools-extra/clangd/unittests/IndexTests.cpp

Index: clang-tools-extra/clangd/unittests/IndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/IndexTests.cpp
+++ clang-tools-extra/clangd/unittests/IndexTests.cpp
@@ -119,8 +119,9 @@
   auto Token = std::make_shared();
   std::weak_ptr WeakToken = Token;
 
-  SwapIndex S(llvm::make_unique(
-  SymbolSlab(), RefSlab(), std::move(Token), /*BackingDataSize=*/0));
+  SwapIndex S(llvm::make_unique(SymbolSlab(), RefSlab(),
+  RelationSlab(), std::move(Token),
+  /*BackingDataSize=*/0));
   EXPECT_FALSE(WeakToken.expired());  // Current MemIndex keeps it alive.
   S.reset(llvm::make_unique()); // Now the MemIndex is destroyed.
   EXPECT_TRUE(WeakToken.expired());   // So the token is too.
@@ -132,12 +133,13 @@
   FuzzyFindRequest Req;
   Req.Query = "2";
   Req.AnyScope = true;
-  MemIndex I(Symbols, RefSlab());
+  MemIndex I(Symbols, RefSlab(), RelationSlab());
   EXPECT_THAT(match(I, Req), ElementsAre("2"));
 }
 
 TEST(MemIndexTest, MemIndexLimitedNumMatches) {
-  auto I = MemIndex::build(generateNumSymbols(0, 100), RefSlab());
+  auto I =
+  MemIndex::build(generateNumSymbols(0, 100), RefSlab(), RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "5";
   Req.AnyScope = true;
@@ -152,7 +154,7 @@
 TEST(MemIndexTest, FuzzyMatch) {
   auto I = MemIndex::build(
   generateSymbols({"LaughingOutLoud", "LionPopulation", "LittleOldLady"}),
-  RefSlab());
+  RefSlab(), RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "lol";
   Req.AnyScope = true;
@@ -162,8 +164,8 @@
 }
 
 TEST(MemIndexTest, MatchQualifiedNamesWithoutSpecificScope) {
-  auto I =
-  MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab());
+  auto I = MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab(),
+   RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.AnyScope = true;
@@ -171,8 +173,8 @@
 }
 
 TEST(MemIndexTest, MatchQualifiedNamesWithGlobalScope) {
-  auto I =
-  MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab());
+  auto I = MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab(),
+   RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.Scopes = {""};
@@ -181,7 +183,8 @@
 
 TEST(MemIndexTest, MatchQualifiedNamesWithOneScope) {
   auto I = MemIndex::build(
-  generateSymbols({"a::y1", "a::y2", "a::x", "b::y2", "y3"}), RefSlab());
+  generateSymbols({"a::y1", "a::y2", "a::x", "b::y2", "y3"}), RefSlab(),
+  RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.Scopes = {"a::"};
@@ -190,7 +193,8 @@
 
 TEST(MemIndexTest, MatchQualifiedNamesWithMultipleScopes) {
   auto I = MemIndex::build(
-  generateSymbols({"a::y1", "a::y2", "a::x", "b::y3", "y3"}), RefSlab());
+  generateSymbols({"a::y1", "a::y2", "a::x", "b::y3", "y3"}), RefSlab(),
+  RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.Scopes = {"a::", "b::"};
@@ -198,7 +202,8 @@
 }
 
 TEST(MemIndexTest, NoMatchNestedScopes) {
-  auto I = MemIndex::build(generateSymbols({"a::y1", "a::b::y2"}), RefSlab());
+  auto I = MemIndex::build(generateSymbols({"a::y1", "a::b::y2"}), RefSlab(),
+   RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.Scopes = {"a::"};
@@ -206,7 +211,8 @@
 }
 
 TEST(MemIndexTest, IgnoreCases) {
-  auto I = MemIndex::build(generateSymbols(

[PATCH] D62839: [clangd] Index API and implementations for relations

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/index/Index.h:107
+  virtual void
+  relations(const SymbolID &Subject, index::SymbolRole Predicate,
+llvm::function_ref Callback) const = 0;

kadircet wrote:
> We might need additional options, like limiting number of results. Could you 
> instead accept a `RelsRequest` object? You can check `RefsRequest` for a 
> sample
I called it `RelationsRequest` to avoid visual confusion with `RefsRequest`.



Comment at: clang-tools-extra/clangd/index/MemIndex.h:29
   this->Refs.try_emplace(R.first, R.second.begin(), R.second.end());
+RelationSlab::Builder Builder;
+for (const Relation &R : Relations)

kadircet wrote:
> why not just store `densemap<,arrayref>` ?
I used `std::vector` because it wasn't clear where the array would live 
otherwise.



Comment at: clang-tools-extra/clangd/index/Merge.cpp:126
+llvm::function_ref Callback) const {
+  // Return results from both indexes but avoid duplicates.
+  llvm::DenseSet SeenObjects;

kadircet wrote:
> avoiding deduplicates is not enough, we also wouldn't want to report stale 
> relations coming from static index.
> 
> Could you check the logic in `MergedIndex::refs`, and hopefully refactor it 
> into a class to share between these two?
The description in `MergedIndex::refs()` says:

```
  // We don't want duplicated refs from the static/dynamic indexes,
  // and we can't reliably deduplicate them because offsets may differ slightly.
  // We consider the dynamic index authoritative and report all its refs,
  // and only report static index refs from other files.
```

It seems to me that the problem of "can't reliably deduplicate them because 
offsets may differ slightly" doesn't apply to relations, as there are no 
offsets involved. So, is there still a need to have additional logic here?

If so, what would the logic be -- don't return an object O1 from the static 
index if we've returned an object O2 from the dynamic index defined in the same 
file as O1? (Note that to implement this, we'd have to additionally look up 
each SymbolID we return in the symbol table, as we don't otherwise know what 
file the object is located in.)



Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:28
+RelationSlab Rels) {
   auto Size = Symbols.bytes() + Refs.bytes();
+  // There is no need to include "Rels" in Data because the relations are self-

kadircet wrote:
> `+rels.size()`
`Size` is only the backing data size, so if we don't include the relations in 
the backing data, we shouldn't include `Rels.size()`, right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62839/new/

https://reviews.llvm.org/D62839



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62839: [clangd] Index API and implementations for relations

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:250
   Path, llvm::make_unique(std::move(Symbols)),
-  llvm::make_unique(), /*CountReferences=*/false);
+  llvm::make_unique(), llvm::make_unique(),
+  /*CountReferences=*/false);

kadircet wrote:
> I think we need to pass relationslab in here. Since we might miss relations 
> like the following that are outside the main file:
> ```
> class A {};
> class B : public A {};
> ```
> Would be glad if you could prove me right/wrong with a unittest as well.
You are right! Fixed, and added a test to FileIndexTests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62839/new/

https://reviews.llvm.org/D62839



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62839: [clangd] Index API and implementations for relations

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 204189.
nridge marked 9 inline comments as done.
nridge added a comment.

Address remaining review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62839/new/

https://reviews.llvm.org/D62839

Files:
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/index/FileIndex.cpp
  clang-tools-extra/clangd/index/FileIndex.h
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/IndexAction.cpp
  clang-tools-extra/clangd/index/IndexAction.h
  clang-tools-extra/clangd/index/MemIndex.cpp
  clang-tools-extra/clangd/index/MemIndex.h
  clang-tools-extra/clangd/index/Merge.cpp
  clang-tools-extra/clangd/index/Merge.h
  clang-tools-extra/clangd/index/Relation.h
  clang-tools-extra/clangd/index/Serialization.cpp
  clang-tools-extra/clangd/index/dex/Dex.cpp
  clang-tools-extra/clangd/index/dex/Dex.h
  clang-tools-extra/clangd/indexer/IndexerMain.cpp
  clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/DexTests.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/IndexActionTests.cpp
  clang-tools-extra/clangd/unittests/IndexTests.cpp

Index: clang-tools-extra/clangd/unittests/IndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/IndexTests.cpp
+++ clang-tools-extra/clangd/unittests/IndexTests.cpp
@@ -119,8 +119,9 @@
   auto Token = std::make_shared();
   std::weak_ptr WeakToken = Token;
 
-  SwapIndex S(llvm::make_unique(
-  SymbolSlab(), RefSlab(), std::move(Token), /*BackingDataSize=*/0));
+  SwapIndex S(llvm::make_unique(SymbolSlab(), RefSlab(),
+  RelationSlab(), std::move(Token),
+  /*BackingDataSize=*/0));
   EXPECT_FALSE(WeakToken.expired());  // Current MemIndex keeps it alive.
   S.reset(llvm::make_unique()); // Now the MemIndex is destroyed.
   EXPECT_TRUE(WeakToken.expired());   // So the token is too.
@@ -132,12 +133,13 @@
   FuzzyFindRequest Req;
   Req.Query = "2";
   Req.AnyScope = true;
-  MemIndex I(Symbols, RefSlab());
+  MemIndex I(Symbols, RefSlab(), RelationSlab());
   EXPECT_THAT(match(I, Req), ElementsAre("2"));
 }
 
 TEST(MemIndexTest, MemIndexLimitedNumMatches) {
-  auto I = MemIndex::build(generateNumSymbols(0, 100), RefSlab());
+  auto I =
+  MemIndex::build(generateNumSymbols(0, 100), RefSlab(), RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "5";
   Req.AnyScope = true;
@@ -152,7 +154,7 @@
 TEST(MemIndexTest, FuzzyMatch) {
   auto I = MemIndex::build(
   generateSymbols({"LaughingOutLoud", "LionPopulation", "LittleOldLady"}),
-  RefSlab());
+  RefSlab(), RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "lol";
   Req.AnyScope = true;
@@ -162,8 +164,8 @@
 }
 
 TEST(MemIndexTest, MatchQualifiedNamesWithoutSpecificScope) {
-  auto I =
-  MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab());
+  auto I = MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab(),
+   RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.AnyScope = true;
@@ -171,8 +173,8 @@
 }
 
 TEST(MemIndexTest, MatchQualifiedNamesWithGlobalScope) {
-  auto I =
-  MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab());
+  auto I = MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab(),
+   RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.Scopes = {""};
@@ -181,7 +183,8 @@
 
 TEST(MemIndexTest, MatchQualifiedNamesWithOneScope) {
   auto I = MemIndex::build(
-  generateSymbols({"a::y1", "a::y2", "a::x", "b::y2", "y3"}), RefSlab());
+  generateSymbols({"a::y1", "a::y2", "a::x", "b::y2", "y3"}), RefSlab(),
+  RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.Scopes = {"a::"};
@@ -190,7 +193,8 @@
 
 TEST(MemIndexTest, MatchQualifiedNamesWithMultipleScopes) {
   auto I = MemIndex::build(
-  generateSymbols({"a::y1", "a::y2", "a::x", "b::y3", "y3"}), RefSlab());
+  generateSymbols({"a::y1", "a::y2", "a::x", "b::y3", "y3"}), RefSlab(),
+  RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.Scopes = {"a::", "b::"};
@@ -198,7 +202,8 @@
 }
 
 TEST(MemIndexTest, NoMatchNestedScopes) {
-  auto I = MemIndex::build(generateSymbols({"a::y1", "a::b::y2"}), RefSlab());
+  auto I = MemIndex::build(generateSymbols({"a::y1", "a::b::y2"}), RefSlab(),
+   RelationSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
   Req.Scopes = {"a::"};
@@ -206,7 +211,8 @@
 }
 
 TEST(MemIndexTest, IgnoreCases) {
-  auto I = MemIndex::build(generateSymbols({"ns::ABC", "ns::abc"