| Message ID | m3aa6sxyfh.fsf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Dec 16, 2011 at 10:40 AM, Dodji Seketeli <dodji@redhat.com> wrote: > Hello, > So I am thinking that maybe letting expand_default_init represent the > invalid initialization would prevent us from getting into this case. > For that to work I had to make > build_constexpr_constructor_member_initializers try a little bit > harder to represent invalid member initialization, as that code wasn't > expecting to have e.g, an EXPR_STMT for a member initialization. Ideally, we should have erroneous member inits be initialized with error_mark_node. > > If you think I am trying too hard, maybe I could just get out early > from register_constexpr_fundef if errorcount is non-zero? Like, right > before: > > if (DECL_CONSTRUCTOR_P (fun) > && cx_check_missing_mem_inits (fun, body, !DECL_GENERATED_P (fun))) > return NULL; > > ? > > The reason why I am not doing this straight away is that I am thinking > that maybe G++ could be missing some diagnostic in that case. And > more generally, I think that representing invalid input (as much as we > can) in the semantic tree is consistent with what's done in the rest > of the FE and leads to better diagnostics. We don't have a good track record at invalid input around for too long :-) Plus, keeping invalid input around for too long means that other part of the front-end gets messed up with hairy conditionals and control flows. I would think that an erroneous member initialized already got a diagnostic and that should suffice and we shouldn't keep trying further...
On 12/16/2011 11:40 AM, Dodji Seketeli wrote: > /* It's OK to skip a member with a trivial constexpr ctor. > A constexpr ctor that isn't trivial should have been > added in by now. */ > gcc_checking_assert (!TYPE_HAS_COMPLEX_DFLT (ftype)); > > If you think I am trying too hard, maybe I could just get out early > from register_constexpr_fundef if errorcount is non-zero? Let's just check errorcount in this assert. > [1]: By the way, I am just curious, why using gcc_checking_assert > instead of just gcc_assert? In general, I think it makes sense to use gcc_checking_assert for checks that either are expensive, or check conditions that aren't really problematic to deal with if they do occur. But I haven't been particularly methodical about using one or the other. Jason
Gabriel Dos Reis <gdr@integrable-solutions.net> writes: > On Fri, Dec 16, 2011 at 10:40 AM, Dodji Seketeli <dodji@redhat.com> wrote: >> Hello, > >> So I am thinking that maybe letting expand_default_init represent the >> invalid initialization would prevent us from getting into this case. >> For that to work I had to make >> build_constexpr_constructor_member_initializers try a little bit >> harder to represent invalid member initialization, as that code wasn't >> expecting to have e.g, an EXPR_STMT for a member initialization. > > Ideally, we should have erroneous member inits be initialized with > error_mark_node. With that patch, the EXPR_STMT wraps an error_mark_node. But yes, I agree. > We don't have a good track record at invalid input around for too long :-) > Plus, keeping invalid input around for too long means that other part > of the front-end gets messed up with hairy conditionals and control flows. > I would think that an erroneous member initialized already got a > diagnostic and that should suffice and we shouldn't keep trying further... OK. Thanks.
diff --git a/gcc/cp/init.c b/gcc/cp/init.c index e93e82c..c958c29 100644 --- a/gcc/cp/init.c +++ b/gcc/cp/init.c @@ -1671,7 +1671,12 @@ expand_default_init (tree binfo, tree true_exp, tree exp, tree init, int flags, } /* FIXME put back convert_to_void? */ - if (TREE_SIDE_EFFECTS (rval)) + if (TREE_SIDE_EFFECTS (rval) + /* If the initialization failed, let's generate an EXPR_STMT + containing error_mark_node, effectively telling other parts + of the compiler that there actually was an initialization, + even though it was invalid. */ + || rval == error_mark_node) finish_expr_stmt (rval); } diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c index 2f2a26a..a8ea996 100644 --- a/gcc/cp/semantics.c +++ b/gcc/cp/semantics.c @@ -5900,7 +5900,18 @@ build_constexpr_constructor_member_initializers (tree type, tree body) } } else - gcc_assert (errorcount > 0); + { + /* An error happened earlier, and some diagnostic has probably + been emitted for it already. */ + gcc_assert (errorcount > 0); + /* What else but an expression could we have at this point? */ + gcc_assert (TREE_CODE_CLASS (TREE_CODE (body)) == tcc_expression); + /* Let's build the data member initialization representation, + even if some initializers might actually contain + error_mark_node. */ + if (ok) + ok = build_data_member_initialization (body, &vec); + } if (ok) return build_constructor (type, vec); else diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-99.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-99.C new file mode 100644 index 0000000..13a5ea3 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-99.C @@ -0,0 +1,13 @@ +// Origin PR c++/51462 +// { dg-options "-std=c++11" } + +struct A +{ + int i = 0; +}; + +struct B +{ + A a; + constexpr B() : a(0) {} // { dg-error "no matching function" } +};
Hello, In the example of the patch below, the invalid initialization for the 'a' member in the context of the constructor of B (in expand_default_init) yields an error_mark_node, and rightfully so. But then expand_default_init doesn't generate any (expression) statement to represent that invalid initialization. So it's just like if no initialization was written by the user. Later, register_constexpr_fundef massages the body of the constexpr constructor B and calls cx_check_missing_mem_inits to see if there are some members that are missing initialization. As expand_default_init didn't generate any statement for the invalid initialization of 'a', the assert below, from cx_check_missing_mem_inits is violated[1]: if (type_has_constexpr_default_constructor (ftype)) { /* It's OK to skip a member with a trivial constexpr ctor. A constexpr ctor that isn't trivial should have been added in by now. */ gcc_checking_assert (!TYPE_HAS_COMPLEX_DFLT (ftype)); So I am thinking that maybe letting expand_default_init represent the invalid initialization would prevent us from getting into this case. For that to work I had to make build_constexpr_constructor_member_initializers try a little bit harder to represent invalid member initialization, as that code wasn't expecting to have e.g, an EXPR_STMT for a member initialization. If you think I am trying too hard, maybe I could just get out early from register_constexpr_fundef if errorcount is non-zero? Like, right before: if (DECL_CONSTRUCTOR_P (fun) && cx_check_missing_mem_inits (fun, body, !DECL_GENERATED_P (fun))) return NULL; ? The reason why I am not doing this straight away is that I am thinking that maybe G++ could be missing some diagnostic in that case. And more generally, I think that representing invalid input (as much as we can) in the semantic tree is consistent with what's done in the rest of the FE and leads to better diagnostics. FWIW, the patch below bootstraps and passes regression tests on x86_64-unknown-linux-gnu against trunk. [1]: By the way, I am just curious, why using gcc_checking_assert instead of just gcc_assert? Thanks. gcc/cp/ PR c++/51462 * init.c (expand_default_init): Pass even error_mark_node to finish_expr_stmt. * semantics.c (build_constexpr_constructor_member_initializers): call build_data_member_initialization even for invalid input. gcc/testsuite/ PR c++/51462 * g++.dg/cpp0x/constexpr-99.C: New test. --- gcc/cp/init.c | 7 ++++++- gcc/cp/semantics.c | 13 ++++++++++++- gcc/testsuite/g++.dg/cpp0x/constexpr-99.C | 13 +++++++++++++ 3 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-99.C