rjmccall added a comment.
Thanks, just a few minor comment requests now.
================
Comment at: include/clang/AST/DeclBase.h:1453
+ /// copy.
+ uint64_t HasNonTrivialToPrimitiveCopyCUnion : 1;
+
----------------
Please include in these comments that these imply the associated basic
non-triviality predicates.
================
Comment at: include/clang/AST/Type.h:1133
+ /// Check if this is or contains a non-trivial C struct/union type.
+ bool hasNonTrivialPrimitiveCStruct() const;
----------------
rjmccall wrote:
> rjmccall wrote:
> > ahatanak wrote:
> > > rjmccall wrote:
> > > > You only want these checks to trigger on unions with non-trivial
> > > > members (or structs containing them), right? How about something like
> > > > `hasNonTrivialPrimitiveCUnionMember()`? Or maybe make it more
> > > > descriptive for the use sites, like `isPrimitiveCRestrictedType()`?
> > > >
> > > > Also, it would be nice if the fast path of this could be inlined so
> > > > that clients usually didn't had to make a call at all. You can write
> > > > the `getBaseElementTypeUnsafe()->getAs<RecordType>()` part in an
> > > > `inline` implementation at the bottom this file.
> > > Since we don't keep track of whether a struct or union is or contains
> > > unions with non-trivial members, we'll have to use the visitors to detect
> > > such structs or unions or, to do it faster, add a bit to `RecordDeclBits`
> > > that indicates the presence of non-trivial unions. I guess it's okay to
> > > add another bit to `RecordDeclBits`?
> > It looks like there's plenty of space in `RecordDeclBits`, yeah.
> This comment seems like the right place to explain what makes a union
> non-trivial in C (that it contains a member which is non-trivial for *any* of
> the reasons that a type might be non-trivial).
Okay, if we're tracking these separately, please put separate comments on each.
Also, please mention in each comment that this implies the associated basic
non-triviality predicate.
================
Comment at: lib/Sema/SemaDecl.cpp:12053
+ NTCUC_UninitAutoVar);
}
+
----------------
ahatanak wrote:
> rjmccall wrote:
> > ahatanak wrote:
> > > rjmccall wrote:
> > > > Please add a comment explaining why this is specific to local variables.
> > > I was trying to explain why this should be specific to local variables
> > > and realized that it's not clear to me whether it should be.
> > >
> > > Suppose there is a union with two fields that are both non-trivial:
> > >
> > > ```
> > > union U {
> > > Type A a;
> > > Type B a;
> > > };
> > >
> > > U global;
> > > ```
> > >
> > > In this case, is value-initialization (which is essentially
> > > default-initialization plus a bunch of zero-initialization as per our
> > > previous discussion) used to initialize `global`? If so, should we reject
> > > the code since it requires default-initialization? It should be fine if
> > > we can assume default-initialization means zero-initialization for
> > > non-trivial types in C, but what if `TypeA` or `TypeB` requires
> > > initializing to a non-zero value?
> > Yeah, the default-initialization dimension of this problem is interesting.
> > The C++ rule makes sense for C++ because default initialization of a C++
> > class requires an actual, arbitrary-side-effects constructor call, which of
> > course you can't reasonably do implicitly for a union member. As discussed
> > previously, non-trivial C types can presumably always be
> > default-initialized with a constant bit pattern. That means that, as long
> > as we can do any initialization work at all, then it's in principle not a
> > problem as long as the bit pattern is the same for all the union members
> > requiring non-trivial initialization (and in particular if there's only one
> > such member). So it's just like you say, we *could* just initialize such
> > unions conservatively as long as two different members don't require
> > inconsistent patterns, which in practice they currently never do. That's
> > all true independent of storage duration — if we can write that pattern
> > into a global, we can write into a local. The only caveat is that a
> > semantic need for non-trivial default initialization almost certainly means
> > that there's a semantic need for non-trivial destruction as well, which of
> > course can't be done on a local union (but isn't a problem for a global
> > because we just don't destroy them).
> >
> > On the other hand, on a language level it's much simpler to just say that
> > we can't default-initialize a union of any storage duration if it has a
> > non-trivial member, and then the language rule doesn't depend on bit-level
> > representations. If there's interest, we can look into weakening that rule
> > later by saying that e.g. it's possible to default-initialize a union with
> > at most one non-trivial member.
> >
> > Apropos, do we consider unions with non-trivial members to be non-trivial
> > members for the purposes of enclosing unions? Seems like we should.
> > Probably the most sensible way to handle that is to also flag the union as
> > being non-trivial in a dimension if it has a member that's non-trivial in
> > that dimension (which might also let you fast-path some of the checking you
> > need to do). Essentially, we'd consider the case where copying is
> > impossible to be a subset of the case where copying is non-trivial.
> Yes, this patch does consider unions with non-trivial members to be
> non-trivial members for the purposes of enclosing unions.
>
> I've made changes that make clang diagnose global variables that are or have
> C union types that are non-trivial to default-initialize. This disallows
> declaring global C union variables that have ObjC ARC pointer fields, but we
> can relax this later if users want them.
Well, presumably you're only diagnosing them if they're actually
default-initialized. Users have an easy workaround if they actually want to
declare a global union containing a `__strong` reference: they can just
initialize the member they actually want to initialize.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63753/new/
https://reviews.llvm.org/D63753
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
