[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Thanks, that looks great.  A few more requests and then this will be ready to go, I think.



================
Comment at: include/clang/AST/DeclBase.h:1454
   /// Number of non-inherited bits in RecordDeclBitfields.
   enum { NumRecordDeclBits = 11 };
 
----------------
This needs to be updated.


================
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:
> 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).


================
Comment at: include/clang/AST/Type.h:1238
+  /// Check if \param RD is or contains a non-trivial C union.
+  bool hasNonTrivialPrimitiveCUnionMember(const RecordDecl *RD) const;
 };
----------------
This should be `static`, right?


================
Comment at: lib/Sema/SemaDecl.cpp:11648
+    if (VDecl->getType().hasNonTrivialPrimitiveCUnionMember() &&
+        VDecl->hasLocalStorage()) {
+      if (auto *ILE = dyn_cast<InitListExpr>(Init))
----------------
Please add a comment explaining why this is specific to local variables.


================
Comment at: lib/Sema/SemaDecl.cpp:12053
+                            NTCUC_UninitAutoVar);
     }
+
----------------
Please add a comment explaining why this is specific to local variables.


================
Comment at: lib/Sema/SemaExpr.cpp:6097
+      // non-trivial to copy or default-initialize.
+      checkNonTrivialCUnionInInitList(ILE);
+  }
----------------
Can we extract a common function that checks the initializer expression of a non-trivial C union?  I think there are at least three separate places that do that in this patch, and it's not too hard to imagine that we might want to add more cases to the common analysis.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753