[PATCH] D47623: [clangd] Avoid indexing decls associated with friend decls.
Eric Liu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Jun 1 06:18:52 PDT 2018
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:
> 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.
================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:816
+TEST_F(SymbolCollectorTest, DoNotIndexSymbolsInFriendDecl) {
+ Annotations Header(R"(
+ namespace nx {
----------------
sammccall wrote:
> Can you also test that:
> - if a friend decl (non-definition) comes first, followed by a non-friend decl (non-definition), then the decl *is* indexed. (maybe just drop the definition from foo, since it's otherwise the same as Y)
> - if a friend decl has a definition, and there is no other declaration, then the decl *is* indexed (and the friend decl is canonical)
Done. I have missed the case where friend decl is a definition. Thanks!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47623
More information about the cfe-commits
mailing list