[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Nathan Hawes via Phabricator via cfe-commits]

nathawes accepted this revision.
nathawes added a comment.
This revision is now accepted and ready to land.

Thank you!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58814



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

[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 189703.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Delete extra parens


Repository:
  rC Clang

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

https://reviews.llvm.org/D58814

Files:
  include/clang/Index/IndexSymbol.h
  lib/Index/IndexDecl.cpp
  lib/Index/IndexSymbol.cpp
  lib/Index/IndexingContext.cpp
  test/Index/Core/index-source.cpp
  unittests/Index/IndexTests.cpp

Index: unittests/Index/IndexTests.cpp
===
--- unittests/Index/IndexTests.cpp
+++ unittests/Index/IndexTests.cpp
@@ -58,11 +58,13 @@
   Position WrittenPos;
   Position DeclPos;
   SymbolInfo SymInfo;
+  SymbolRoleSet Roles;
   // FIXME: add more information.
 };
 
 llvm::raw_ostream <<(llvm::raw_ostream , const TestSymbol ) {
-  return OS << S.QName << '[' << S.WrittenPos << ']' << '@' << S.DeclPos;
+  return OS << S.QName << '[' << S.WrittenPos << ']' << '@' << S.DeclPos << '('
+<< static_cast(S.SymInfo.Kind) << ')';
 }
 
 class Indexer : public IndexDataConsumer {
@@ -84,6 +86,7 @@
 S.WrittenPos = Position::fromSourceLocation(Loc, AST->getSourceManager());
 S.DeclPos =
 Position::fromSourceLocation(D->getLocation(), AST->getSourceManager());
+S.Roles = Roles;
 Symbols.push_back(std::move(S));
 return true;
   }
@@ -143,6 +146,7 @@
 MATCHER_P(WrittenAt, Pos, "") { return arg.WrittenPos == Pos; }
 MATCHER_P(DeclAt, Pos, "") { return arg.DeclPos == Pos; }
 MATCHER_P(Kind, SymKind, "") { return arg.SymInfo.Kind == SymKind; }
+MATCHER_P(HasRole, Role, "") { return arg.Roles & static_cast(Role); }
 
 TEST(IndexTest, Simple) {
   auto Index = std::make_shared();
@@ -257,6 +261,32 @@
   Contains(AllOf(QName("std::foo"), Kind(SymbolKind::Using;
 }
 
+TEST(IndexTest, Constructors) {
+  std::string Code = R"cpp(
+struct Foo {
+  Foo(int);
+  ~Foo();
+};
+  )cpp";
+  auto Index = std::make_shared();
+  IndexingOptions Opts;
+  tooling::runToolOnCode(new IndexAction(Index, Opts), Code);
+  EXPECT_THAT(
+  Index->Symbols,
+  UnorderedElementsAre(
+  AllOf(QName("Foo"), Kind(SymbolKind::Struct),
+WrittenAt(Position(2, 12))),
+  AllOf(QName("Foo::Foo"), Kind(SymbolKind::Constructor),
+WrittenAt(Position(3, 7))),
+  AllOf(QName("Foo"), Kind(SymbolKind::Struct),
+HasRole(SymbolRole::NameReference), WrittenAt(Position(3, 7))),
+  AllOf(QName("Foo::~Foo"), Kind(SymbolKind::Destructor),
+WrittenAt(Position(4, 7))),
+  AllOf(QName("Foo"), Kind(SymbolKind::Struct),
+HasRole(SymbolRole::NameReference),
+WrittenAt(Position(4, 8);
+}
+
 } // namespace
 } // namespace index
 } // namespace clang
Index: test/Index/Core/index-source.cpp
===
--- test/Index/Core/index-source.cpp
+++ test/Index/Core/index-source.cpp
@@ -5,17 +5,17 @@
 class Cls { public:
   // CHECK: [[@LINE+3]]:3 | constructor/C++ | Cls | c:@S@Cls@F@Cls#I# | __ZN3ClsC1Ei | Decl,RelChild | rel: 1
   // CHECK-NEXT: RelChild | Cls | c:@S@Cls
-  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont,NameReference | rel: 1
   Cls(int x);
   // CHECK: [[@LINE+2]]:3 | constructor/cxx-copy-ctor/C++ | Cls | c:@S@Cls@F@Cls#&1$@S@Cls# | __ZN3ClsC1ERKS_ | Decl,RelChild | rel: 1
-  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont,NameReference | rel: 1
   Cls(const Cls &);
   // CHECK: [[@LINE+2]]:3 | constructor/cxx-move-ctor/C++ | Cls | c:@S@Cls@F@Cls#&&$@S@Cls# | __ZN3ClsC1EOS_ | Decl,RelChild | rel: 1
-  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont,NameReference | rel: 1
   Cls(Cls &&);
 
   // CHECK: [[@LINE+2]]:3 | destructor/C++ | ~Cls | c:@S@Cls@F@~Cls# | __ZN3ClsD1Ev | Decl,RelChild | rel: 1
-  // CHECK: [[@LINE+1]]:4 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+  // CHECK: [[@LINE+1]]:4 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont,NameReference | rel: 1
   ~Cls();
 };
 
@@ -35,12 +35,12 @@
 Cls::Cls(int x) {}
 // CHECK: [[@LINE-1]]:6 | constructor/C++ | Cls | c:@S@Cls@F@Cls#I# | __ZN3ClsC1Ei | Def,RelChild | rel: 1
 // CHECK: [[@LINE-2]]:1 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
-// CHECK: [[@LINE-3]]:6 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+// CHECK: [[@LINE-3]]:6 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont,NameReference | rel: 1
 
 Cls::~/*a comment*/Cls() {}
 // CHECK: [[@LINE-1]]:6 | destructor/C++ | ~Cls | c:@S@Cls@F@~Cls# | __ZN3ClsD1Ev | Def,RelChild | rel: 1
 // CHECK: [[@LINE-2]]:1 | class/C++ | Cls | c:@S@Cls |  | 

[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

As discussed offline constructor calls are a different issue, coming from even 
a different layer than Indexing API.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58814



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

[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added a comment.

Do we also need to change anything for constructor calls?  If not, please add a 
test for that.




Comment at: lib/Index/IndexDecl.cpp:251
+   Ctor->getParent(), Ctor->getDeclContext(),
+   (unsigned)(SymbolRole::NameReference));
 

No need for parens around `SymbolRole::NameReference`.



Comment at: lib/Index/IndexDecl.cpp:268
+ Dtor->getParent(), Dtor->getDeclContext(),
+ (unsigned)(SymbolRole::NameReference));
   }

No need for parens around `SymbolRole::NameReference`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58814



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

[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 189695.
kadircet added a comment.

- Add NameReference role kind, and tag references from constructors/destructors 
to the class with that kind.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58814

Files:
  include/clang/Index/IndexSymbol.h
  lib/Index/IndexDecl.cpp
  lib/Index/IndexSymbol.cpp
  lib/Index/IndexingContext.cpp
  test/Index/Core/index-source.cpp
  unittests/Index/IndexTests.cpp

Index: unittests/Index/IndexTests.cpp
===
--- unittests/Index/IndexTests.cpp
+++ unittests/Index/IndexTests.cpp
@@ -58,11 +58,13 @@
   Position WrittenPos;
   Position DeclPos;
   SymbolInfo SymInfo;
+  SymbolRoleSet Roles;
   // FIXME: add more information.
 };
 
 llvm::raw_ostream <<(llvm::raw_ostream , const TestSymbol ) {
-  return OS << S.QName << '[' << S.WrittenPos << ']' << '@' << S.DeclPos;
+  return OS << S.QName << '[' << S.WrittenPos << ']' << '@' << S.DeclPos << '('
+<< static_cast(S.SymInfo.Kind) << ')';
 }
 
 class Indexer : public IndexDataConsumer {
@@ -84,6 +86,7 @@
 S.WrittenPos = Position::fromSourceLocation(Loc, AST->getSourceManager());
 S.DeclPos =
 Position::fromSourceLocation(D->getLocation(), AST->getSourceManager());
+S.Roles = Roles;
 Symbols.push_back(std::move(S));
 return true;
   }
@@ -143,6 +146,7 @@
 MATCHER_P(WrittenAt, Pos, "") { return arg.WrittenPos == Pos; }
 MATCHER_P(DeclAt, Pos, "") { return arg.DeclPos == Pos; }
 MATCHER_P(Kind, SymKind, "") { return arg.SymInfo.Kind == SymKind; }
+MATCHER_P(HasRole, Role, "") { return arg.Roles & static_cast(Role); }
 
 TEST(IndexTest, Simple) {
   auto Index = std::make_shared();
@@ -257,6 +261,32 @@
   Contains(AllOf(QName("std::foo"), Kind(SymbolKind::Using;
 }
 
+TEST(IndexTest, Constructors) {
+  std::string Code = R"cpp(
+struct Foo {
+  Foo(int);
+  ~Foo();
+};
+  )cpp";
+  auto Index = std::make_shared();
+  IndexingOptions Opts;
+  tooling::runToolOnCode(new IndexAction(Index, Opts), Code);
+  EXPECT_THAT(
+  Index->Symbols,
+  UnorderedElementsAre(
+  AllOf(QName("Foo"), Kind(SymbolKind::Struct),
+WrittenAt(Position(2, 12))),
+  AllOf(QName("Foo::Foo"), Kind(SymbolKind::Constructor),
+WrittenAt(Position(3, 7))),
+  AllOf(QName("Foo"), Kind(SymbolKind::Struct),
+HasRole(SymbolRole::NameReference), WrittenAt(Position(3, 7))),
+  AllOf(QName("Foo::~Foo"), Kind(SymbolKind::Destructor),
+WrittenAt(Position(4, 7))),
+  AllOf(QName("Foo"), Kind(SymbolKind::Struct),
+HasRole(SymbolRole::NameReference),
+WrittenAt(Position(4, 8);
+}
+
 } // namespace
 } // namespace index
 } // namespace clang
Index: test/Index/Core/index-source.cpp
===
--- test/Index/Core/index-source.cpp
+++ test/Index/Core/index-source.cpp
@@ -5,17 +5,17 @@
 class Cls { public:
   // CHECK: [[@LINE+3]]:3 | constructor/C++ | Cls | c:@S@Cls@F@Cls#I# | __ZN3ClsC1Ei | Decl,RelChild | rel: 1
   // CHECK-NEXT: RelChild | Cls | c:@S@Cls
-  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont,NameReference | rel: 1
   Cls(int x);
   // CHECK: [[@LINE+2]]:3 | constructor/cxx-copy-ctor/C++ | Cls | c:@S@Cls@F@Cls#&1$@S@Cls# | __ZN3ClsC1ERKS_ | Decl,RelChild | rel: 1
-  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont,NameReference | rel: 1
   Cls(const Cls &);
   // CHECK: [[@LINE+2]]:3 | constructor/cxx-move-ctor/C++ | Cls | c:@S@Cls@F@Cls#&&$@S@Cls# | __ZN3ClsC1EOS_ | Decl,RelChild | rel: 1
-  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont,NameReference | rel: 1
   Cls(Cls &&);
 
   // CHECK: [[@LINE+2]]:3 | destructor/C++ | ~Cls | c:@S@Cls@F@~Cls# | __ZN3ClsD1Ev | Decl,RelChild | rel: 1
-  // CHECK: [[@LINE+1]]:4 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+  // CHECK: [[@LINE+1]]:4 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont,NameReference | rel: 1
   ~Cls();
 };
 
@@ -35,12 +35,12 @@
 Cls::Cls(int x) {}
 // CHECK: [[@LINE-1]]:6 | constructor/C++ | Cls | c:@S@Cls@F@Cls#I# | __ZN3ClsC1Ei | Def,RelChild | rel: 1
 // CHECK: [[@LINE-2]]:1 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
-// CHECK: [[@LINE-3]]:6 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+// CHECK: [[@LINE-3]]:6 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont,NameReference | rel: 1
 
 Cls::~/*a comment*/Cls() {}
 // CHECK: [[@LINE-1]]:6 | destructor/C++ | ~Cls | c:@S@Cls@F@~Cls# | __ZN3ClsD1Ev | Def,RelChild | rel: 1
 // CHECK: 

[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Nathan Hawes via Phabricator via cfe-commits]

nathawes added a comment.
Herald added a subscriber: ormris.

In D58814#1415607 , @gribozavr wrote:

> > These references were added to support using the index data for symbol 
> > rename.
>
> Understood -- thank you for providing the context (the original commit that 
> added this code didn't have the explanation why this reference was added).


That was my patch (via Argyrios), so sorry about that!

> However, my suggestion would be to still take this patch.  For symbol rename, 
> my suggestion would be one of the following.
> 
> Option (1): Symbol rename is a complex operation that requires knowing many 
> places where the class name appears.  So I think it is fair that it would 
> need to navigate to all such declarations using the semantic index -- find 
> the class, then find its constructors, destructor, out-of-line functions, 
> find derived classes, then find `using` declarations in derived classes, find 
> calls to constructors, destructor etc.  I don't think it is not the job of 
> the core indexer API to hardcode all of these places as a "reference to the 
> class".  Different clients require different navigation to related symbols, 
> and hardcoding all such navigation patterns in the indexer would create a 
> messy index that is difficult to work with, since you'd have to filter out 
> lots of stuff.  Not to even mention the index size.

I feel like the complexity you mention here is what makes it worthwhile to 
expose all these locations as occurrences of the type, and I'm fairly sure 
we're actually already doing that in all these locations (unless I've missed 
other patches removing them). Pushing this work onto all clients that want to 
provide automatic global rename functionality or to support users manually 
renaming by including these in their find-references results seems like a 
poorer outcome from a code sharing/maintenance point of view than continuing to 
provide it and requiring clients that don't want it to check the SymbolRole 
bits inside their IndexDataConsumer's handleDeclOccurrence (`if (Roles & 
SymbolRole::NameReference) return true;`). I don't think the index size is 
concerning; clients don't have to store these occurrences if they don't care 
about them.

> Option (2): A client that wants to hardcode all navigation patterns in the 
> index can still do this -- in the `IndexDataConsumer`, it is possible to 
> handle the `ConstructorDecl` as a reference to the class, if desired.  The 
> flow of the indexing information becomes more clear in my opinion: when there 
> is a constructor decl in the source, the `IndexDataConsumer` gets one 
> `ConstructorDecl`. Then the specific implementation of `IndexDataConsumer` 
> decides to treat it also as a class reference, because it wants to support a 
> specific approach to performing symbol renaming.

I think the downside here is again the maintenance burden. This code would 
probably live downstream somewhere so whenever changes happen upstream that 
affect the code deriving these extra occurrences in clients that want it, or 
introduce new types of locations that those clients don't handle and miss, 
there's more duplicated effort updating/hunting down bugs. I also personally 
see these as legitimate occurrences of the type rather than hardcoded 
navigation patterns (see below), even though rename was the motivation here.

> As is, the indexing information is misleading and surprising.  When we see a 
> constructor decl or a constructor call, there is no reference to the class at 
> that point -- there is a reference to a constructor.  I think index is 
> supposed to expose semantic information, not just that there's a token in the 
> source code that has the same spelling as the class name.
> 
>> could we instead distinguish these cases with a more specific SymbolRole, 
>> e.g. NameReference as opposed to Reference, and filter them based on that 
>> instead?
> 
> It feels like a workaround to me, that also pushes the fix to the clients of 
> the indexing API. The core of the problem still exists: the indexing 
> information is surprising, and requires extra work on the client to make it 
> not surprising (filtering out NameReferences).

I agree that most people think of it as purely being the constructor name, but 
I don't think the semantic connection to the class is really as loose as 'a 
token with the same spelling' and I think it's reasonable to report it. To 
resolve a constructor call, the compiler looks for a *type* with that spelling. 
It's not as if the user chooses to name their constructor with the same name as 
the type – the name lookup finds the type. One interesting consequence of this 
is that the compiler seems to complain if you try to reference the constructor 
as opposed to the type when you call it, e.g.

  class A {
  public:
A(int x) {}
  };
  
  int main() {
auto x = A(2); // ok
auto y = A::A(2); // error: qualified reference to 'A' is a constructor 

[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

I agree with Dmitri's reasoning. I believe this should be handled inside the 
`renamer`s logic as mentioned in option 1. But if you believe this should not 
be the case, I am also happy to move logic of `adding a reference to class` on 
constructors to the consumer of renamer(as mentioned in option 2).


Repository:
  rC Clang

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

https://reviews.llvm.org/D58814



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

[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> These references were added to support using the index data for symbol rename.

Understood -- thank you for providing the context (the original commit that 
added this code didn't have the explanation why this reference was added).  
However, my suggestion would be to still take this patch.  For symbol rename, 
my suggestion would be one of the following.

Option (1): Symbol rename is a complex operation that requires knowing many 
places where the class name appears.  So I think it is fair that it would need 
to navigate to all such declarations using the semantic index -- find the 
class, then find its constructors, destructor, out-of-line functions, find 
derived classes, then find `using` declarations in derived classes, find calls 
to constructors, destructor etc.  I don't think it is not the job of the core 
indexer API to hardcode all of these places as a "reference to the class".  
Different clients require different navigation to related symbols, and 
hardcoding all such navigation patterns in the indexer would create a messy 
index that is difficult to work with, since you'd have to filter out lots of 
stuff.  Not to even mention the index size.

Option (2): A client that wants to hardcode all navigation patterns in the 
index can still do this -- in the `IndexDataConsumer`, it is possible to handle 
the `ConstructorDecl` as a reference to the class, if desired.  The flow of the 
indexing information becomes more clear in my opinion: when there is a 
constructor decl in the source, the `IndexDataConsumer` gets one 
`ConstructorDecl`. Then the specific implementation of `IndexDataConsumer` 
decides to treat it also as a class reference, because it wants to support a 
specific approach to performing symbol renaming.

As is, the indexing information is misleading and surprising.  When we see a 
constructor decl or a constructor call, there is no reference to the class at 
that point -- there is a reference to a constructor.  I think index is supposed 
to expose semantic information, not just that there's a token in the source 
code that has the same spelling as the class name.

> could we instead distinguish these cases with a more specific SymbolRole, 
> e.g. NameReference as opposed to Reference, and filter them based on that 
> instead?

It feels like a workaround to me, that also pushes the fix to the clients of 
the indexing API. The core of the problem still exists: the indexing 
information is surprising, and requires extra work on the client to make it not 
surprising (filtering out NameReferences).


Repository:
  rC Clang

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

https://reviews.llvm.org/D58814



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

[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Nathan Hawes via Phabricator via cfe-commits]

nathawes requested changes to this revision.
nathawes added a comment.
This revision now requires changes to proceed.

These references were added to support using the index data for symbol rename. 
I.e. so that when you rename the class, you can use the index data to find all 
occurrences of its name, including its use in constructor/destructor 
declarations and references (hence the test cases in 
test/Index/Core/index-source.cpp). If you need to specifically find references 
of the class symbol, as opposed to its name, could we instead distinguish these 
cases with a more specific SymbolRole, e.g. NameReference as opposed to 
Reference, and filter them based on that instead?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58814



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

[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added a comment.
This revision is now accepted and ready to land.

Please also add a test for find references on a constructor.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58814



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

[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added reviewers: akyrtzi, gribozavr.
Herald added subscribers: cfe-commits, arphaman.
Herald added a project: clang.

In current indexing logic we get references to class itself when we see
a constructor/destructor which is not true. Eventough spelling is same with
class' name it does not refer to it. This patch deletes those calls.


Repository:
  rC Clang

https://reviews.llvm.org/D58814

Files:
  lib/Index/IndexDecl.cpp
  test/Index/Core/index-source.cpp
  unittests/Index/IndexTests.cpp

Index: unittests/Index/IndexTests.cpp
===
--- unittests/Index/IndexTests.cpp
+++ unittests/Index/IndexTests.cpp
@@ -257,6 +257,23 @@
   Contains(AllOf(QName("std::foo"), Kind(SymbolKind::Using;
 }
 
+TEST(IndexTest, Constructors) {
+  std::string Code = R"cpp(
+struct Foo {
+  Foo(int);
+  ~Foo();
+};
+  )cpp";
+  auto Index = std::make_shared();
+  IndexingOptions Opts;
+  tooling::runToolOnCode(new IndexAction(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols,
+  UnorderedElementsAre(
+  AllOf(QName("Foo"), Kind(SymbolKind::Struct)),
+  AllOf(QName("Foo::Foo"), Kind(SymbolKind::Constructor)),
+  AllOf(QName("Foo::~Foo"), Kind(SymbolKind::Destructor;
+}
+
 } // namespace
 } // namespace index
 } // namespace clang
Index: test/Index/Core/index-source.cpp
===
--- test/Index/Core/index-source.cpp
+++ test/Index/Core/index-source.cpp
@@ -5,17 +5,17 @@
 class Cls { public:
   // CHECK: [[@LINE+3]]:3 | constructor/C++ | Cls | c:@S@Cls@F@Cls#I# | __ZN3ClsC1Ei | Decl,RelChild | rel: 1
   // CHECK-NEXT: RelChild | Cls | c:@S@Cls
-  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+
   Cls(int x);
   // CHECK: [[@LINE+2]]:3 | constructor/cxx-copy-ctor/C++ | Cls | c:@S@Cls@F@Cls#&1$@S@Cls# | __ZN3ClsC1ERKS_ | Decl,RelChild | rel: 1
-  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+
   Cls(const Cls &);
   // CHECK: [[@LINE+2]]:3 | constructor/cxx-move-ctor/C++ | Cls | c:@S@Cls@F@Cls#&&$@S@Cls# | __ZN3ClsC1EOS_ | Decl,RelChild | rel: 1
-  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+
   Cls(Cls &&);
 
   // CHECK: [[@LINE+2]]:3 | destructor/C++ | ~Cls | c:@S@Cls@F@~Cls# | __ZN3ClsD1Ev | Decl,RelChild | rel: 1
-  // CHECK: [[@LINE+1]]:4 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+
   ~Cls();
 };
 
@@ -35,12 +35,10 @@
 Cls::Cls(int x) {}
 // CHECK: [[@LINE-1]]:6 | constructor/C++ | Cls | c:@S@Cls@F@Cls#I# | __ZN3ClsC1Ei | Def,RelChild | rel: 1
 // CHECK: [[@LINE-2]]:1 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
-// CHECK: [[@LINE-3]]:6 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
 
 Cls::~/*a comment*/Cls() {}
 // CHECK: [[@LINE-1]]:6 | destructor/C++ | ~Cls | c:@S@Cls@F@~Cls# | __ZN3ClsD1Ev | Def,RelChild | rel: 1
 // CHECK: [[@LINE-2]]:1 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
-// CHECK: [[@LINE-3]]:20 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
 
 template 
 class TemplCls {
@@ -394,7 +392,6 @@
 // CHECK: [[@LINE-1]]:3 | constructor/cxx-copy-ctor/C++ | DeletedMethods | c:@S@DeletedMethods@F@DeletedMethods#&1$@S@DeletedMethods# | __ZN14DeletedMethodsC1ERKS_ | Def,RelChild | rel: 1
 // CHECK: RelChild | DeletedMethods | c:@S@DeletedMethods
 // CHECK: [[@LINE-3]]:24 | struct/C++ | DeletedMethods | c:@S@DeletedMethods |  | Ref,RelCont | rel: 1
-// CHECK: [[@LINE-4]]:3 | struct/C++ | DeletedMethods | c:@S@DeletedMethods |  | Ref,RelCont | rel: 1
 };
 
 namespace ns2 {
@@ -494,7 +491,7 @@
 // CHECK: [[@LINE-1]]:69 | variable/C++ | structuredBinding2 | c:@N@cpp17structuredBinding@structuredBinding2 |  | Ref,Read,RelCont | rel: 1
 // CHECK-NEXT: RelCont | localStructuredBindingAndRef | c:@N@cpp17structuredBinding@F@localStructuredBindingAndRef#
 // CHECK-NOT: localBinding
-// LOCAL: [[@LINE-4]]:9 | variable(local)/C++ | localBinding1 | c:index-source.cpp@25408@N@cpp17structuredBinding@F@localStructuredBindingAndRef#@localBinding1
+// LOCAL: [[@LINE-4]]:9 | variable(local)/C++ | localBinding1 | c:index-source.cpp@24750@N@cpp17structuredBinding@F@localStructuredBindingAndRef#@localBinding1
 }
 
 }
Index: lib/Index/IndexDecl.cpp
===
--- lib/Index/IndexDecl.cpp
+++ lib/Index/IndexDecl.cpp
@@ -246,9 +246,6 @@
 handleDeclarator(D);
 
 if (const CXXConstructorDecl *Ctor = dyn_cast(D)) {
-  IndexCtx.handleReference(Ctor->getParent(), Ctor->getLocation(),
-   Ctor->getParent(), Ctor->getDeclContext());
-
   // Constructor initializers.
   for (const auto *Init : Ctor->inits()) {
 if (Init->isWritten()) {
@@ -259,12 +256,6 @@
   IndexCtx.indexBody(Init->getInit(), D, D);
 }
   }
-} else if