On Sun, Jan 3, 2016 at 3:14 PM, Martin Sebor <mse...@gmail.com> wrote: > On 12/31/2015 08:40 AM, Patrick Palka wrote: >> >> If we do create such an initializer, we end up with an error_mark_node >> during gimplification, because in cp-gimplify.c we pass this >> VEC_INIT_EXPR of the flexible array member to build_vec_init, for which >> it spits on an error_mark_node. This happens in e.g. the test case >> g++.dg/ext/array1.C. >> >> This patch makes it so that we avoid generating an initializer for a >> flexible array member, thus avoiding to end up with an error_mark_node >> during gimplification. The same kind of thing is done ~90 lines down >> from the code I changed. > > > I don't think this change is correct (or complete). We could > decide that the kind of code it's meant to allow should be > accepted in general but if we did, this change alone would not > be sufficient. > > In the way of background, the existing special treatment for > flexible array members was added to prevent rejecting cases > like this one: > > struct A { A (int); }; > struct B { > B (); > int n; > A a[]; > }; > > Since B's ctor above doesn't initialize the flexible array > member it's well-formed. Ditto if B's ctor is defined but > avoids initializing B::a. > > The proposed change has the effect of also accepting the > following modified version of the example above that is > currently rejected: > > struct A { A (int); }; > struct B { > B (): n (), a () { } > int n; > A a[]; > }; > > In this modified example, B's ctor attempts to default-initialize > the flexible array member and its elements using their respective > default ctors even though no such ctor for the latter exists. > > Since C++ flexible array members are a GCC extension whose C++ > specific aspects are essentially undocumented it's open to > discussion whether or not code like this should be accepted. > Clang rejects it, but one could make the argument that since > the flexible array member has no elements, default initializing > it should be allowed even if the same initialization would be > ill-formed if the array wasn't empty. > > If the code should be accepted as written above then it should > also be accepted if B's ctor were omitted or defaulted. I.e., > the following should be treated the same as the above: > > struct A { A (int); }; > struct B { > // ditto with B() = default; > int n; > A a[]; > } b; > > The patch doesn't handle either of these cases and the code > is rejected both with and without it.
Good catch. I had not intended to change the validity of certain uses of flexible array members with this patch. So at least for now it seems that we should not avoid building a VEC_INIT_EXPR of a flexible array member, since calling build_vec_init_expr is necessary to diagnose some invalid uses of flexible array members. So, I see three possible solutions to avoid propagating an error_mark_node to the gimplifier while preserving the exact behavior of flexible array members: Continue to unconditionally call_build_vec_init_expr() in perform_member_init(), and 1. Don't add the resulting VEC_INIT_EXPR to the statement tree if the array in question has no upper bound (i.e. is a flexible array member); or 2. Modify build_vec_init() to return an empty statement instead of returning error_mark_node, if the array to be initialized has no upper bound (and its initializer is NULL_TREE); or 3. Modify cp_gimplify_expr() to avoid calling build_vec_init() if the VEC_INIT_EXPR in question is for an array with no upper bound (and its initializer is NULL_TREE). Which approach is best? The third one is the most conservative and the one least likely to cause inadvertent changes in behavior elsewhere, I think. (In the end however, this "bug" is mostly harmless, and a patch fixing it is really only useful in combination with patch #3/3 to avoid a spurious ICE-on-valid.)