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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked an inline comment as done.
ahatanak added inline comments.


================
Comment at: lib/Sema/SemaExpr.cpp:16218
+    checkNonTrivialCUnion(E->getType(), E->getExprLoc(),
+                          Sema::NTCUC_LValueToRValueVolatile);
+
----------------
rjmccall wrote:
> It looks like you're generally warning about this based on the specific context that forced an lvalue-to-rvalue conversion.  I'm not sure `volatile` is special except that we actually perform the load even in unused-value contexts.  Is the assumption that you've exhaustively covered all the other contexts of lvalue-to-rvalue conversions whose values will actually be used?  That seems questionable to me.
Yes, that was my assumption. All the other contexts where lvalue-to-rvalue conversion is performed and the result is used are already covered by other calls sites of `checkNonTrivialCUnion`, which informs the users that the struct/union is being used in an invalid context.

Do you have a case in mind that I didn't think of where a lvalue-to-rvalue conversion requires a non-trivial initialization/destruction/copying of a union but clang fails to emit any diagnostics?

Also I realized that lvalue-to-rvalue conversion of volatile types doesn't always require non-trivial destruction, so I think `CheckDestruct` shouldn't be set in this case.

```
void foo(U0 *a, volatile U0 *b) {
  // this doesn't require destruction.
  // this is perfectly valid if U0 is non-trivial to destruct but trivial to copy.
  *a = *b;  
}
```

For the same reason, I think `CheckDestruct` shouldn't be set for function returns (but it should be set for function parameters since they are destructed by the callee).


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753