rjmccall added inline comments.
================
Comment at: lib/Sema/SemaExpr.cpp:16218
+ checkNonTrivialCUnion(E->getType(), E->getExprLoc(),
+ Sema::NTCUC_LValueToRValueVolatile);
+
----------------
ahatanak wrote:
> 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).
There are a *lot* of places that trigger lvalue-to-rvalue conversion. Many of
them aren't legal with structs (in C), but I'm worried about approving a
pattern with the potential to be wrong by default just because we didn't think
about some weird case. As an example, casts can trigger lvalue-to-rvalue
conversion; I think the only casts allowed with structs are the identity cast
and the cast to `void`, but those are indeed allowed. Now, a cast to `void`
means the value is ignored, so we can elide a non-volatile load in the operand,
and an identity cast isn't terminal; if the idea is that we're checking all the
*terminal* uses of a struct r-value, then we're in much more restricted
territory (and don't need to worry about things like commas and conditional
operators that can propagate values out). But this still worries me.
I'm not sure we need to be super-pedantic about destructibility vs. copyability
in some of this checking. It's certain possible in C++, but I can't imagine
what sort of *primitive* type would be trivially copyable but not trivially
destructible. (The reverse isn't true: something like a relative pointer would
be non-trivially copyable but still trivially destructible.)
Is there any way to make this check cheaper so that we can immediately avoid
any further work for types that are obviously copyable/destructible? All the
restricted types are (possibly arrays of) record types, right?
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
