| Message ID | 4FCC8F97.3030504@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/04/2012 06:36 AM, Florian Weimer wrote: > (Sorry if Thunderbird has garbled the changelog entries.) I add the ChangeLog to the top of the patch to avoid this. :) > - if (TREE_CODE (w) != INTEGER_CST) > + if (w == error_mark_node) > + ; /* Continue with error processing below. */ > + else if (TREE_CODE (w) != INTEGER_CST) > - error ("enumerator value for %qD is not an integer constant", > - name); > + if (value != error_mark_node) > + error ("enumerator value for %qD is not an integer constant", > + name); Hmm, I don't see these errors as redundant, but rather as giving context for the previous error. > + else if (!parser->integral_constant_expression_p > + && !parser->non_integral_constant_expression_p) > + expression = error_mark_node; This doesn't make sense to me. parser->integral_constant_expression_p should always be true at this point if you're moving the restore later (which also seems unnecessary). Jason
On 06/04/2012 08:40 PM, Jason Merrill wrote: > On 06/04/2012 06:36 AM, Florian Weimer wrote: >> (Sorry if Thunderbird has garbled the changelog entries.) > > I add the ChangeLog to the top of the patch to avoid this. :) Good idea. >> - if (TREE_CODE (w) != INTEGER_CST) >> + if (w == error_mark_node) >> + ; /* Continue with error processing below. */ >> + else if (TREE_CODE (w) != INTEGER_CST) > >> - error ("enumerator value for %qD is not an integer constant", >> - name); >> + if (value != error_mark_node) >> + error ("enumerator value for %qD is not an integer constant", >> + name); > > Hmm, I don't see these errors as redundant, but rather as giving context > for the previous error. Even with caret diagnostics? Anyway, I will try to resolve this in a different way. >> + else if (!parser->integral_constant_expression_p >> + && !parser->non_integral_constant_expression_p) >> + expression = error_mark_node; > > This doesn't make sense to me. parser->integral_constant_expression_p > should always be true at this point if you're moving the restore later > (which also seems unnecessary). I think parser->integral_constant_expression_p reflects the result of the cp_parser_assignment_expression() call earlier in this function.
>>> - error ("enumerator value for %qD is not an integer constant", >>> - name); >>> + if (value != error_mark_node) >>> + error ("enumerator value for %qD is not an integer constant", >>> + name); >> >> >> Hmm, I don't see these errors as redundant, but rather as giving context >> for the previous error. > > > Even with caret diagnostics? caret is optional. > Anyway, I will try to resolve this in a different way.
On 06/04/2012 04:12 PM, Florian Weimer wrote: >> This doesn't make sense to me. parser->integral_constant_expression_p >> should always be true at this point if you're moving the restore later >> (which also seems unnecessary). > > I think parser->integral_constant_expression_p reflects the result of > the cp_parser_assignment_expression() call earlier in this function. parser->integral_constant_expression_p is set to indicate that the current context expects an integral constant expression; the call to cp_parser_assignment_expression will not affect that. If the expression turns out not to be a valid constant expression, then we set parser->non_integral_constant_expression_p to true, but we don't touch parser->integral_constant_expression_p. I think the condition you want is if (parser->non_integral_constant_expression_p && !allow_non_constant_p) Jason
On 06/04/2012 10:46 PM, Jason Merrill wrote: > On 06/04/2012 04:12 PM, Florian Weimer wrote: >>> This doesn't make sense to me. parser->integral_constant_expression_p >>> should always be true at this point if you're moving the restore later >>> (which also seems unnecessary). >> >> I think parser->integral_constant_expression_p reflects the result of >> the cp_parser_assignment_expression() call earlier in this function. > > parser->integral_constant_expression_p is set to indicate that the > current context expects an integral constant expression; the call to > cp_parser_assignment_expression will not affect that. > > If the expression turns out not to be a valid constant expression, then > we set parser->non_integral_constant_expression_p to true, but we don't > touch parser->integral_constant_expression_p. Okay, now all this makes sense. This is a bit difficult to figure out just reading the comments in cp/parser.h. > I think the condition you want is > > if (parser->non_integral_constant_expression_p > && !allow_non_constant_p) True. But if we want cascading errors, cp_parser_constant_expression really cannot return error_mark_node, so this approach is a dead end. (For example, build_enumerator replaces error_mark_node in the enumeration value with nothing, i.e., the next possible enumeration value.) So this approach is a dead end. On the other hand, if cascading errors are acceptable, I probably should not worry too much about them in operator new, either. 8-)
On 06/05/2012 11:19 AM, Florian Weimer wrote: > True. But if we want cascading errors, cp_parser_constant_expression > really cannot return error_mark_node, so this approach is a dead end. > (For example, build_enumerator replaces error_mark_node in the > enumeration value with nothing, i.e., the next possible enumeration value.) > > So this approach is a dead end. On the other hand, if cascading errors > are acceptable, I probably should not worry too much about them in > operator new, either. 8-) It really depends. The error messages I was talking about before add more context information to the the previous error, which is good. Better would be to have set a constant expression context somewhere so that we can cover both the context and the violation in one error message. In the operator new case, the non-constant expression error followed by the VLA warning is not as helpful, as the latter ignores the former. Perhaps the right way to deal with this is to allow non-constant expressions in the new-type-id, since we allow them in a regular type-id. Jason
On Tue, Jun 5, 2012 at 12:05 PM, Jason Merrill <jason@redhat.com> wrote: > In the operator new case, the non-constant expression error followed by the > VLA warning is not as helpful, as the latter ignores the former. Perhaps the > right way to deal with this is to allow non-constant expressions in the > new-type-id, since we allow them in a regular type-id. and afterward issue the diagnostics. That sounds good to me :-) -- Gaby
Index: gcc/testsuite/g++.dg/warn/overflow-warn-4.C =================================================================== --- gcc/testsuite/g++.dg/warn/overflow-warn-4.C (revision 188104) +++ gcc/testsuite/g++.dg/warn/overflow-warn-4.C (working copy) @@ -20,11 +20,9 @@ enum e { /* { dg-error "enumerator value for 'E4' is not an integer constant" "enum error" { xfail *-*-* } 19 } */ E5 = INT_MAX + 1, /* { dg-warning "integer overflow in expression" } */ /* { dg-error "overflow in constant expression" "constant" { target *-*-* } 21 } */ - /* { dg-error "enumerator value for 'E5' is not an integer constant" "enum error" { target *-*-* } 21 } */ /* Again, overflow in evaluated subexpression. */ E6 = 0 * (INT_MAX + 1), /* { dg-warning "integer overflow in expression" } */ - /* { dg-error "overflow in constant expression" "constant" { target *-*-* } 25 } */ - /* { dg-error "enumerator value for 'E6' is not an integer constant" "enum error" { target *-*-* } 25 } */ + /* { dg-error "overflow in constant expression" "constant" { target *-*-* } 24 } */ /* A cast does not constitute overflow in conversion. */ E7 = (char) INT_MAX }; @@ -33,8 +31,7 @@ struct s { int a; int : 0 * (1 / 0); /* { dg-warning "division by zero" } */ int : 0 * (INT_MAX + 1); /* { dg-warning "integer overflow in expression" } */ - /* { dg-error "overflow in constant expression" "constant" { target *-*-* } 35 } */ - /* { dg-error "bit-field .* width not an integer constant" "" { target *-*-* } 35 } */ + /* { dg-error "overflow in constant expression" "constant" { target *-*-* } 33 } */ }; void @@ -56,10 +53,10 @@ void *n = 0; constants. The third has the overflow in an unevaluated subexpression, so is a null pointer constant. */ void *p = 0 * (INT_MAX + 1); /* { dg-warning "integer overflow in expression" } */ -/* { dg-error "invalid conversion from 'int' to 'void" "null" { target *-*-* } 58 } */ +/* { dg-error "invalid conversion from 'int' to 'void" "null" { target *-*-* } 55 } */ void *q = 0 * (1 / 0); /* { dg-warning "division by zero" } */ -/* { dg-error "invalid conversion from 'int' to 'void*'" "null" { xfail *-*-* } 61 } */ +/* { dg-error "invalid conversion from 'int' to 'void*'" "null" { xfail *-*-* } 58 } */ void *r = (1 ? 0 : INT_MAX+1); /* { dg-bogus "integer overflow in expression" "" { xfail *-*-* } } */ void @@ -70,7 +67,7 @@ g (int i) case 0 * (1/0): /* { dg-warning "division by zero" } */ ; case 1 + 0 * (INT_MAX + 1): /* { dg-warning "integer overflow in expression" } */ - /* { dg-error "overflow in constant expression" "constant" { target *-*-* } 72 } */ + /* { dg-error "overflow in constant expression" "constant" { target *-*-* } 69 } */ ; } } Index: gcc/testsuite/g++.dg/cpp0x/regress/error-recovery1.C =================================================================== --- gcc/testsuite/g++.dg/cpp0x/regress/error-recovery1.C (revision 188104) +++ gcc/testsuite/g++.dg/cpp0x/regress/error-recovery1.C (working copy) @@ -7,5 +7,3 @@ foo () const bool b =; // { dg-error "" } foo < b > (); // { dg-error "constant expression" } }; - -// { dg-error "no match" "" { target *-*-* } 8 } Index: gcc/testsuite/g++.dg/cpp0x/pr51225.C =================================================================== --- gcc/testsuite/g++.dg/cpp0x/pr51225.C (revision 188104) +++ gcc/testsuite/g++.dg/cpp0x/pr51225.C (working copy) @@ -12,3 +12,5 @@ template<typename> struct bar { static constexpr A<1> b = A<1>(x); // { dg-error "not declared" } }; + +// { dg-error "template argument.*invalid" "" { target *-*-* } 8 } Index: gcc/cp/init.c =================================================================== --- gcc/cp/init.c (revision 188104) +++ gcc/cp/init.c (working copy) @@ -2805,6 +2805,34 @@ build_new (VEC(tree,gc) **placement, tree type, tr make_args_non_dependent (*init); } + /* If the type is variably modified (a GNU extension for C + compatibility), we could end up with a variable-times-variable + multiplication at run time, complicating overflow checking. */ + if (variably_modified_type_p (type, NULL_TREE)) + { + /* Try to transform new (T[n]) to new T[n], and accept the + result if T is not variably modified. */ + bool good = false; + if (nelts == NULL_TREE && TREE_CODE (type) == ARRAY_TYPE) + { + tree inner_type = TREE_TYPE (type); + if (!variably_modified_type_p (inner_type, NULL_TREE)) + { + pedwarn (input_location, OPT_Wvla, + "ISO C++ forbids variable array length in parenthesized type-id"); + nelts = array_type_nelts_top (type); + type = inner_type; + good = true; + } + } + if (!good) + { + if (complain & tf_error) + error ("new cannot be applied to a variably modified type"); + return error_mark_node; + } + } + if (nelts) { if (!build_expr_type_conversion (WANT_INT | WANT_ENUM, nelts, false)) Index: gcc/cp/class.c =================================================================== --- gcc/cp/class.c (revision 188104) +++ gcc/cp/class.c (working copy) @@ -2902,7 +2902,9 @@ check_bitfield_decl (tree field) w = cxx_constant_value (w); input_location = loc; - if (TREE_CODE (w) != INTEGER_CST) + if (w == error_mark_node) + ; /* Continue with error processing below. */ + else if (TREE_CODE (w) != INTEGER_CST) { error ("bit-field %q+D width not an integer constant", field); w = error_mark_node; Index: gcc/cp/decl.c =================================================================== --- gcc/cp/decl.c (revision 188104) +++ gcc/cp/decl.c (working copy) @@ -12453,8 +12453,9 @@ build_enumerator (tree name, tree value, tree enum if (TREE_CODE (value) != INTEGER_CST || ! INTEGRAL_OR_ENUMERATION_TYPE_P (TREE_TYPE (value))) { - error ("enumerator value for %qD is not an integer constant", - name); + if (value != error_mark_node) + error ("enumerator value for %qD is not an integer constant", + name); value = NULL_TREE; } } Index: gcc/cp/parser.c =================================================================== --- gcc/cp/parser.c (revision 188104) +++ gcc/cp/parser.c (working copy) @@ -7689,9 +7689,6 @@ cp_parser_constant_expression (cp_parser* parser, determine whether a particular assignment-expression is in fact constant. */ expression = cp_parser_assignment_expression (parser, /*cast_p=*/false, NULL); - /* Restore the old settings. */ - parser->integral_constant_expression_p - = saved_integral_constant_expression_p; parser->allow_non_integral_constant_expression_p = saved_allow_non_integral_constant_expression_p; if (cxx_dialect >= cxx0x) @@ -7701,11 +7698,17 @@ cp_parser_constant_expression (cp_parser* parser, separately in e.g. cp_parser_template_argument. */ bool is_const = potential_rvalue_constant_expression (expression); parser->non_integral_constant_expression_p = !is_const; - if (!is_const && !allow_non_constant_p) - require_potential_rvalue_constant_expression (expression); + if (!is_const && !allow_non_constant_p + && !require_potential_rvalue_constant_expression (expression)) + expression = error_mark_node; } + else if (!parser->integral_constant_expression_p + && !parser->non_integral_constant_expression_p) + expression = error_mark_node; if (allow_non_constant_p) *non_constant_p = parser->non_integral_constant_expression_p; + parser->integral_constant_expression_p + = saved_integral_constant_expression_p; parser->non_integral_constant_expression_p = saved_non_integral_constant_expression_p;
Without this change, cp_parser_constant_expression reports errors in C++11 mode, but does not provide any indication to callers, which continue processing and produce misleading errors. The changes to check_bitfield_decl and build_enumerator are required for identical error report in C++11 and C++98 mode. This removes several spurious errors, and the testsuite is adjusted accordingly. Bootstrapped on x86_64-linux-gnu with C, C++, TLO enabled, "make check-c++" passes with no regressions. (Sorry if Thunderbird has garbled the changelog entries.) 2012-06-04 Florian Weimer <fweimer@redhat.com> * parser.c (cp_parser_constant_expression): Return error_mark_node on error if !allow_non_constant_p. * class.c (check_bitfield_decl): Handle error_mark_node in width. * decl.c (build_enumerator): Handle error_mark_node in value. 2012-06-04 Florian Weimer <fweimer@redhat.com> * g++.dg/warn/overflow-warn-4.C, g++.dg/cpp0x/pr51225.C, g++.dg/cpp0x/regress/error-recovery1.C: Adjust error messages.