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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D47623#1118951, @ilya-biryukov wrote:

> In https://reviews.llvm.org/D47623#1118810, @sammccall wrote:
>
> > - friend decls that are not definitions should be ignored for indexing purposes
>
>
> This is not generally true IIUC. A friend declaration can be a reference, a declaration or a definition.
>
>   int foo(int);
>   int bar(int, int);
>   class ExistingFwdCls;
>  
>   class X {
>     friend class ExistingFwdCls; // a reference and a declaration.
>     friend class NewClass; // a new declaration.
>     friend int foo(int); // a reference and a declaration.
>     friend int baz(int, int, int); // a new declaration.
>   };
>  
>   class Y {
>     friend class ::ExistingFwdCls; // qualified => just a reference.
>     friend int ::bar(int a, int b); // qualified => just a reference.
>     friend int foo(int) { // a reference  and a definition
>       return 100;
>     }
>   };
>
>
> Note that friend functions with bodies are probably ok as canonical declaration, as they are often the only declaration, e.g.
>
>   class Foo {
>     friend bool operator < (Foo const& lhs, Foo const&lhs) {
>      return false;
>     }
>   };
>


Sorry for the late response Ilya. I was trying to test these cases. So, with the current change, if a real "canonical" declaration comes before the friend decl, then the reference will still be recorded (I've added a test case for this). Would this address your concern?



================
Comment at: clangd/index/SymbolCollector.cpp:303
+  // canonical declaration in the index.
+  if (D->getFriendObjectKind() != Decl::FriendObjectKind::FOK_None)
+    D = ASTNode.OrigD;
----------------
sammccall wrote:
> Ah, now I understand - we're picking another to use as canonical. But the exception for definitions should apply here too. And there's nothing special about *OrigD* that makes it a good pick for canonical.
> 
> For determinism, can we instead iterate over the redecls of D and pick the first one that's not a friend or is a definition?
> 
> (I'd pull that check out into a function `shouldSkipDecl` and rename the existing one `shouldSkipSymbol`, but up to you)
> But the exception for definitions should apply here too
I'm not sure why this is needed. Do you have an example?

> And there's nothing special about *OrigD* that makes it a good pick for canonical.
I think `OrigD` is "special" because we have checked that it's not an uninteresting friend decl.

> For determinism, can we instead iterate over the redecls of D and pick the first one that's not a friend or is a definition?
This seems to be the most ideal option, although, in practice, it's a bit hard to check if each `redecl` is a definition, as we don't have `Roles` for them. To make it more deterministic, I added a map from clang's canonical decl to clangd's "canonical" decl so that we could be sure that all redecls would have the same `D`. WDYT?

>(I'd pull that check out into a function shouldSkipDecl and rename the existing one shouldSkipSymbol, but up to you)
I gave this a try, but it couldn't find a very good abstraction. Happy to chat more next week.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47623