[PATCH] D62988: Add an attribute to allow fields of non-trivial types in C unions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D62988#1540359 <https://reviews.llvm.org/D62988#1540359>, @rjmccall wrote:

> For ARC, you could bzero the union member; this is what how we tell people to initialize malloc'ed memory as well, since there's no default-constructor concept that they can invoke for such cases.
>  Our immediate need for this attribute is that we have some code that wants to adopt non-trivial annotations in unions, with no intention of ever copying or destroying them as aggregates.


OK, so we'd be looking at something like this when using the attribute:

  typedef struct S {
    int is_b;
    union {
      float f;
      __attribute__((non_trivial_union_member)) id b;
    };
  } S;
  S make() {
    S s = {0, 0.0f};
    return s;
  }
  void set_f(S *s, float f) {
    if (s->is_b) s->b = 0;
    s->is_b = 0;
    s->f = f;
  }
  void set_b(S *s, id b) {
    if (!s->is_b) bzero(s, sizeof(S));
    s->is_b = 1;
    s->b = b;
  }
  void destroy_s(S *s) {
    if (s->is_b) s->b = 0;
  }

versus the same code written without the attribute, which might be:

  typedef struct S {
    int is_b;
    union {
      float f;
      void *b;
    };
  } S;
  S make() {
    S s = {0, 0.0f};
    return s;
  }
  void set_f(S *s, float f) {
    if (s->is_b) (__bridge_transfer id)s->b;
    s->is_b = 0;
    s->f = f;
  }
  void set_b(S *s, id b) {
    if (s->is_b) (__bridge_transfer id)s->b;
    s->is_b = 1;
    s->b = (__bridge_retained void*)b;
  }
  void destroy_s(S *s) {
    if (s->is_b) (__bridge_transfer id)s->b;
  }

To me, using the attribute here doesn't seem worthwhile; the first form of this code seems scary since important reference counting actions are hidden behind what appear to be dead stores, and the second form (while verbose) is at least explicit about what's going on. If you still think this is a useful feature, so be it, but we should at least be explicit in Clang's ARC documentation about how to correctly make use of it.

I'm not sure whether we should support this as a way of disabling the union triviality features in general, or only the restriction on lifetime types, but either way, I think the support of this attribute should not be dependent on the language mode; I think adding a C-only extension for this does a disservice to our users.

> Of course a better solution would be to make any C code that would copy or destroy the aggregate type illegal, but that seems like a big project.  But maybe there'd be no need for this attribute in the long term if we eventually do have that support.

Yeah, maybe so; adopting the C++11 "unrestricted unions" behavior for non-trivial types in unions in C does seem to make a certain amount of sense.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62988