[PATCH] D47623: [clangd] Avoid indexing decls associated with friend decls.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/index/SymbolCollector.cpp:297
+    // If OrigD is an object of a friend declaration, skip it.
+    if (ASTNode.OrigD->getFriendObjectKind() !=
+        Decl::FriendObjectKind::FOK_None)
----------------
ioeric wrote:
> sammccall wrote:
> > this seems suspect, we're going to treat the third decl in `friend X; X; friend X` differently from that in `X; friend X; friend X;`.
> > 
> > Why? i.e. why is the inner check necessary and why does it treat the original (meaning first, I think) decl specially?
> > this seems suspect, we're going to treat the third decl in `friend X; X; friend X` differently from that in `X; friend X; friend X;`.
> I'm not very sure if I understand the problem. But I'll try to explain what would happen for these two examples.
> - For the first example, the first friend decl will be the canonical decl, and we would only index the second `X` since its `OrigD` is not in friend decl. Both the first and third friend decl will not be indexed. 
> - For the second example, the first non-friend `X` will be the canonical decl, and all three occurrences will have the same `D` pointing to it. This probably means that the same X will be processed three times, but it's probably fine (we might want to optimize it). 
> 
> Basically, `D` is always the canonical declaration in AST and `OrigD` is  the declaration that the indexer is currently visiting. I agree it's confusing...
> 
> > Why? i.e. why is the inner check necessary and why does it treat the original (meaning first, I think) decl specially?
> 
> The inner check handles the following case:
> ```
> class X {
>   friend void foo();
> }
> void foo();
> ```
> 
> There will be two occurrences of `foo` in the index:
> 1. The friend decl, where `D` will be the canonical decl (i.e. friend foo) and `OrigD` will also be friend foo.
> 2. The non-friend decl, where `D` will still be the canonical decl (i.e. friend foo) and `OrigD` is now the non-friend decl.
> Basically, D is always the canonical declaration in AST and OrigD is the declaration that the indexer is currently visiting. I agree it's confusing...

Whoops, I had this backwards. So I guess I mean the outer check.
Fundamentally my question is: in an *arbitrary* list of redecls of a symbol, what is special about the canonical declaration (D, which is just the first in the list) that we particularly care whether it's a friend?
I would expect that either we're ignoring the other redecls, or we're looping over all redecls.

And here, I think we could just skip all friend decls (that are not definitions), regardless of what `D->getFriendObjectKind()` is. If we have other decls, the symbol was indexed already. If we do not, then we don't want to index it anyway.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47623