[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