[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: 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;
----------------
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).


================
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:
> 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?


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256