[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 inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:633
+  "capture a variable of type %1}3 "
+  "since it %select{contains a union that |}2is non-trivial to "
+  "%select{default-initialize|destruct|copy}0">;
----------------
Should this be "`%select{contains|is}2 a union that is non-trivial to..."?  None of these situations bans non-trivial types, just unions containing them (and aggregates containing such unions).


================
Comment at: include/clang/Sema/Sema.h:2116
 
+  // Contexts where using non-trivial C union types can be disallowed.
+  enum NonTrivialCUnionContext {
----------------
If this is parallel with the diagnostic, I'd suggest mentioning that in the comment.


================
Comment at: include/clang/Sema/Sema.h:2121
+    // Function return.
+    NTCUC_FuncitonReturn,
+    // Uninialized variable with automatic storage duration.
----------------
Same typo in both of these.


================
Comment at: include/clang/Sema/Sema.h:2126
+    NTCUC_AutoVar,
+    // Initializer expression for object with automatic storage duration.
+    NTCUC_AutoObjInit,
----------------
Please expand this comment to clarify that this it's only used when we (might be) initializing an object by copying another.

Why is this specific to automatic storage duration?  Just because C doesn't allow other initializers to be non-constant in the first place?


================
Comment at: lib/Sema/SemaDecl.cpp:11085
+    if (isa<ImplicitValueInitExpr>(I))
+      continue;
+    if (auto E = dyn_cast<InitListExpr>(I))
----------------
Why is this okay?  Don't we need to check default-initialization for these?


================
Comment at: lib/Sema/SemaDecl.cpp:11089
+    else
+      checkNonTrivialCUnion(I->getType(), I->getExprLoc(), NTCUC_AutoObjInit);
+  }
----------------
Please include a comment like:

```
  // Assume all other initializers involving copying some existing object.
  // TODO: ignore any initializers where we can guarantee copy-elision
```


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753