rsmith added a comment. In D62988#1539230 <https://reviews.llvm.org/D62988#1539230>, @ahatanak wrote:
> In D62988#1538577 <https://reviews.llvm.org/D62988#1538577>, @rsmith wrote: > > > How do you write correct (non-leaking, non-double-freeing, > > non-releasing-invalid-pointers) code with this attribute? For example, > > suppose I have a `__strong` union member: does storing to it release the > > old value (which might be a different union member)? If so, how do you work > > around that? > > https://clang.llvm.org/docs/AutomaticReferenceCounting.html#ownership-qualified-fields-of-structs-and-unions > > should be updated to say what happens. If manual reference counting code > > is required to make any use of this feature correct (which seems > > superficially likely), is this a better programming model than > > `__unsafe_unretained`? > > > I should first make it clear that this attribute is a dangerous feature that > makes it easier for users to write incorrect code if they aren't careful. > > To write correct code, users have to manually (I don't mean > manual-retain-release, this is compiled with ARC) do assignments to the > fields that are annotated with the attribute to patch up the code generated > by the compiler since the compiler isn't generating the kind of special > functions it generates for non-trivial structs. For example: > > union U1 { > id __weak __attribute__((non_trivial_union_member)) a; > id __attribute__((non_trivial_union_member)) b; > }; > > id getObj(int); > > void foo() { > union U1 u1 = { .b = 0}; // zero-initialize 'b'. > u1.b = getObj(1); // assign to __strong field 'b'. > u1.b = getObj(2); // retain and assign another object to 'b' and release > the previously referenced object. This is what I expected and what I was worried about. I could be missing something, but this approach appears to me to cause this feature to not be useful in many cases. If a store to a union member now first reads from (and releases) that union member, then you cannot change the active union member to a member with lifetime type, because that would trigger a load of an inactive union member with undefined behavior. For example, consider: union U { float f; __attribute__((non_trivial_union_member)) id b; }; void f() { union U u = {.f = 1.0f}; u.b = getObj(1); // undefined behavior: tries to release existing value of u.b, which is not active union member; the implied `[u.b release]` is going to do Very Bad Things. In C, storing to a union member is the way you change the active member, and I can't off-hand think of any sensible workaround for this problem. But maybe I overlooked it: can you demonstrate how you would correctly write the last line of the above example? If that can't be done reasonably, I do not think we should add this attribute. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62988/new/ https://reviews.llvm.org/D62988 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
