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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
