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

[Eric Liu via Phabricator via cfe-commits]

ioeric 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)
----------------
sammccall wrote:
> 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.
> 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?
It's because we have chosen to use `D` as a symbol's canonical declaration (line 332). Here we want to override the canonical declaration `D` when it's a friend decl, so that the first non-friend decl would become the canonical declaration of the symbol.

> And here, I think we could just skip all friend decls (that are not definitions), regardless of what D->getFriendObjectKind() is. 
OK. I think I understand where the confusion came from now. The override of `D` should've been a separate check:
```
if (OrigD is non-definition friend)
  skip;
if (D is friend decl)
  D = OrigD;
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47623