[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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/AST/ASTContext.h:2060
+  /// attr::ObjCOwnership implies an ownership qualifier was explicitly
+  /// specified rather than being added implicitly by the compiler.
+  bool isObjCOwnershipAttributedType(QualType Ty) const;
----------------
ahatanak wrote:
> rjmccall wrote:
> > How about something like: "Return true if the type has been explicitly qualified with ObjC ownership.  A type may be implicitly qualified with ownership under ObjC ARC, and in some cases the compiler treats these differently".
> > 
> > Could you look for other places where we look for an explicit qualifier?  I'm pretty sure I remember that happening once or twice.
> I found that there is a function named `hasDirectOwnershipQualifier` in SemaType.cpp which also detects explicit ownership qualifiers, so I'm using that instead of `isObjCOwnershipAttributedType`. The difference between `isObjCOwnershipAttributedType` is that it detects paren types like `I*(__strong x)` and doesn't look through typedefs (see the test case in non-trivial-c-union.h).
Okay.  I don't know what the right rule here is, but I agree we should be using the same rule in both places.


================
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);
----------------
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?


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256