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:
> 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?
I'm not sure I fully understand what you are saying, but by "cheaper", do you
mean fewer and simpler rules for allowing or disallowing non-trivial unions
even when that might result in rejecting unions used in contexts in which
non-trivial initialization/destruction/copying is not required? If so, we can
probably diagnose any lvalue-to-rvalue conversions regardless of whether the
source is volatile if the type is either non-trivial to copy or destruct.
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
