[PATCH] D131685: [clang][AST] RecursiveASTVisitor should visit owned TagDecl of friend type.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

This looks like a reasonable representation of such record decls to me.

Changing the AST to nest them under typelocs instead is indeed a bigger project (and it's hard to say whether it's a good idea without a lot of digging).



================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1577-1578
 DEF_TRAVERSE_DECL(FriendTemplateDecl, {
+  // FIXME: Traverse getOwnedTagDecl like at the FriendDecl case?
+  // (FriendTemplateDecl is not used)
   if (D->getFriendType())
----------------
davrec wrote:
> I don't think anything is necessary here, because we should never see an `OwnedTagDecl` here.
> In the FriendDecl case the ElaboratedType only has an OwnedTagDecl when `struct Fr` has not yet been declared before the friend decl.
> In the documentation of FriendTemplateDecl on the other hand these examples are given:
> ```
> /// template \<typename T> class A {
> ///   friend class MyVector<T>; // not a friend template
> ///   template \<typename U> friend class B; // not a friend template
> ///   template \<typename U> friend class Foo<T>::Nested; // friend template
> /// };
> ```
> So, a FriendTemplateDecl is only created when referencing a nested class template member.  But that *must* have been declared before the friend decl, or we will get an error:
> 
> ```
> template<typename T> struct B;
> template<typename T> struct A { template<typename U> friend struct B<T>::Fr; }; //ERROR: no member named 'Fr' in B<T>
> ```
> 
> So the OwnedTagDecl should always be nullptr here/the issue should never arise in this case.
> 
That sounds plausible to me, FriendTemplateDecl is extremely obscure and possibly badly designed.
I'd probably err on the side of not adding a FIXME unless we're sure something needs to be done, as I doubt anyone will ever remove it.


================
Comment at: clang/unittests/AST/ASTContextParentMapTest.cpp:145
+  EXPECT_THAT(Ctx.getParents(FrBLoc), ElementsAre(DynTypedNode::create(FrB)));
+  if (FrATagDecl)
+    EXPECT_THAT(Ctx.getParents(*FrATagDecl),
----------------
I'm confused why this is conditional: isn't this the main thing that we're testing? Why do we want the test to silently pass if the AST structure changes so that ownedTagDecl is null? It's hard to tell from reading the code if anything is being tested at all.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131685/new/

https://reviews.llvm.org/D131685