On Sat, Feb 16, 2019 at 08:51:33AM -1000, Jason Merrill wrote:
> > The likely case is still that nothing has changed in between, so this patch
> > just quickly verifies if that is the case (by comparing
> > CONSTRUCTOR_ELT (ctor, 0) with the previously saved value of that and by
> > checking if at the spot in the vector is the expected index). If that is
> > the case, it doesn't do anything else, otherwise it updates the valp
> > pointer.
>
> For scalar types, as in all your testcases, we can evaluate the initializer
> before the target, as C++17 wants. We probably still need your patch for
> when type is a class.
If you are ok that the scalar vs. aggregate case will be handled
differently, I'm all for your patch, though I guess instead of that second
hunk it should change:
if (AGGREGATE_TYPE_P (type) || VECTOR_TYPE_P (type))
into:
if (!preeval)
and move the init = cxx_eval_constant_expression ... call into the
body of that if. I guess that means the scalar store will be handled right
even for unions then. Just wonder if similar to
if (*non_constant_p)
return t;
after target = cxx_eval_... we shouldn't have that for (both) init =
cxx_eval_... cases too.
The testcases can be all changed to work with say struct Z { int z; };
instead of int (or any other aggregate) and I think my patch or something
similar is needed.
With unions, I think the most nasty case is when the union member to which
we want to store is active before an assignment, but is then made inactive
and later active again.
struct Z { int x, y; };
union W { Z a; long long w; };
W w {};
w.a = { 5, 0 }; // w.a becomes the active member
w.a = { (int) (w.w = 17LL + w.a.x), 2 };
So, if we don't preevaluate init, we look up w.a as { 5, 0 } active member
and try to store that, but in the meantime the init evaluation changes
active member to something different, which should invalidate w.a.
Jakub
