[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: lib/Sema/SemaDecl.cpp:11176
     for (const FieldDecl *FD : RD->fields())
-      asDerived().visit(FD->getType(), FD, InNonTrivialUnion);
+      if (!FD->hasAttr<UnavailableAttr>())
+        asDerived().visit(FD->getType(), FD, InNonTrivialUnion);
----------------
rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > Can we make these exceptions only apply to the attributes we synthesize rather than arbitrary uses of `__attribute__((unavailable))`?  These aren't really good semantics in general.
> > > 
> > > You can do the check in a well-named function like `isSuppressedNonTrivialMember`, which would be a good place for a comment explaining what's going on here and why this seemed like the most reasonable solution.
> > We are ignoring unavailable fields here since they don't make the containing union non-trivial and we don't want the user to think that the unavailable field has to be a trivial field in order to fix a compile error. For example, without the check for `UnavailableAttr`, clang will diagnose the unavailable field `f0` when the following code is compiled:
> > 
> > ```
> > union U3 {
> >   id f0 __attribute__((unavailable)); // expected-note {{f0 has type '__strong id' that is non-trivial to default-initialize}}
> >   __weak id f1; // expected-note {{f1 has type '__weak id' that is non-trivial to default-initialize}}
> > };
> > 
> > void test(void) {
> >   union U3 b; // expected-error {{cannot default-initialize an object of type 'union U3' since it is a union that is non-trivial to default-initialize}}
> > }
> > ```
> > 
> > In that case, does it matter whether the field is explicitly annotated unavailable in the source code or implicitly by the compiler?
> Oh, I didn't realize we were already honoring the ordinary attribute in the ABI, but from the code it looks we do.  In that case, yeah, it makes sense to ignore it here.  Should we extract a function to ask whether a `FieldDecl` should be ignored for triviality computations so that it's more self-documenting that there's a consistent logic here?
I added function `shouldDiagnoseNonTrivialField`, which returns false if the field is unavailable.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256