[PATCH] D57438: [Sema][ObjC] Allow declaring ObjC pointer members in C++ unions under ARC

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Asking for a minor tweak, but feel free to commit with that.



================
Comment at: lib/Sema/SemaDeclCXX.cpp:7084
 
+  if (FD->getParent()->isUnion() &&
+      shouldDeleteForVariantObjCPtrMember(FD, FieldType))
----------------
ahatanak wrote:
> rjmccall wrote:
> > rjmccall wrote:
> > > I believe the right check for variant-ness is `inUnion()`, not `FD->getParent()->isUnion()`, since the latter can miss cases with e.g. anonymous `struct`s.
> > Can you try adding a test case here for an anonymous struct nested within an anonymous union?
> Added a test case for an anonymous struct nested within an anonymous union (`struct S2` in arc-0x.mm).
> 
> I found out that the diagnostic messages clang prints for arc-0x.mm are the same whether `inUnion()` or `FD->getParent()->isUnion()` is called. This is what I discovered:
> 
> `SpecialMemberDeletionInfo` is used in two cases:
> 
> 1. To find the deletedness of the special functions in the AST after a class is parsed.
> 
> 2. When a deleted special function is used, provide more details on why the function is deleted and print note diagnostics.
> 
> Since the nested classes are parsed before the enclosing classes, we have all the information we need about the members that are directly contained to correctly infer the deletedness of a class' special functions. So the first case should work fine regardless of which method is called.
> 
> It doesn't make a difference for the second case either because `SpecialMemberDeletionInfo` doesn't try to find out which member of an anonymous struct is causing the special function to be deleted; it only does so for anonymous unions (which happens inside the if statement near comment "// Some additional restrictions exist on the variant members." at line 7140).
Alright.  Another place where we could generally improve diagnostics, then.


================
Comment at: test/SemaObjCXX/arc-0x.mm:174
+    union {
+      struct { // expected-note 6 {{'S2' is implicitly deleted because variant field '' has a non-trivial}}
+        id f0;
----------------
Worth adding a FIXME saying that this note should go on `f1`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57438