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