| Message ID | CABYV9SXOFNBvV6VSGN+B8xx8uXysjS7ThTti2yzm3eFbvhuKjA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 5 Oct 2011, Artem Shinkarov wrote: > On Tue, Oct 4, 2011 at 11:51 PM, Joseph S. Myers > <joseph@codesourcery.com> wrote: > > On Tue, 4 Oct 2011, Artem Shinkarov wrote: > > > >> Hi > >> > >> Here is the patch tho fix bconstp-3.c failure in the bug 50607. The > >> failure was cause because the new parser routine did not consider > >> original_tree_code of the expression. > >> > >> The patch is bootstrapped on x86-unknown-linux-gnu and is being tested. > > > > Please repost the patch for review without the unrelated whitespace > > changes. > > > > -- > > Joseph S. Myers > > joseph@codesourcery.com > > > > Sure. The new version is in the attachment. This version is OK, provided that it has passed bootstrap with no regressions and that it fixes the failures in question.
On Wed, Oct 5, 2011 at 4:22 PM, Joseph S. Myers <joseph@codesourcery.com> wrote: > On Wed, 5 Oct 2011, Artem Shinkarov wrote: > >> On Tue, Oct 4, 2011 at 11:51 PM, Joseph S. Myers >> <joseph@codesourcery.com> wrote: >> > On Tue, 4 Oct 2011, Artem Shinkarov wrote: >> > >> >> Hi >> >> >> >> Here is the patch tho fix bconstp-3.c failure in the bug 50607. The >> >> failure was cause because the new parser routine did not consider >> >> original_tree_code of the expression. >> >> >> >> The patch is bootstrapped on x86-unknown-linux-gnu and is being tested. >> > >> > Please repost the patch for review without the unrelated whitespace >> > changes. >> > >> > -- >> > Joseph S. Myers >> > joseph@codesourcery.com >> > >> >> Sure. The new version is in the attachment. > > This version is OK, provided that it has passed bootstrap with no > regressions and that it fixes the failures in question. > > -- > Joseph S. Myers > joseph@codesourcery.com > Thanks, I am going to apply it then, when the regtesting is over in case it would be successfull. Joseph, is it possible to commit the patch together with the space fixes? Thanks, Artem.
On Wed, 5 Oct 2011, Artem Shinkarov wrote:
> Joseph, is it possible to commit the patch together with the space fixes?
You should not commit whitespace fixes to lines not otherwise modified by
a patch, except in a separate commit that *only* has whitespace or other
formatting fixes, so that "svn blame" results are more meaningful. That
is: commit whitespace fixes (assuming they are genuinely fixes rather than
making things worse) *separately* in a commit whose commit message makes
clear it is just whitespace fixes.
On Wed, Oct 5, 2011 at 5:28 PM, Joseph S. Myers <joseph@codesourcery.com> wrote: > On Wed, 5 Oct 2011, Artem Shinkarov wrote: > >> Joseph, is it possible to commit the patch together with the space fixes? > > You should not commit whitespace fixes to lines not otherwise modified by > a patch, except in a separate commit that *only* has whitespace or other > formatting fixes, so that "svn blame" results are more meaningful. That > is: commit whitespace fixes (assuming they are genuinely fixes rather than > making things worse) *separately* in a commit whose commit message makes > clear it is just whitespace fixes. Ok, I see. In the given patch all the fixes are mainly auto-generated and remove the trailing spaces. But I see your point. Thanks, Artem. > -- > Joseph S. Myers > joseph@codesourcery.com >
On Wed, Oct 5, 2011 at 5:32 PM, Artem Shinkarov <artyom.shinkaroff@gmail.com> wrote: > On Wed, Oct 5, 2011 at 5:28 PM, Joseph S. Myers <joseph@codesourcery.com> wrote: >> On Wed, 5 Oct 2011, Artem Shinkarov wrote: >> >>> Joseph, is it possible to commit the patch together with the space fixes? >> >> You should not commit whitespace fixes to lines not otherwise modified by >> a patch, except in a separate commit that *only* has whitespace or other >> formatting fixes, so that "svn blame" results are more meaningful. That >> is: commit whitespace fixes (assuming they are genuinely fixes rather than >> making things worse) *separately* in a commit whose commit message makes >> clear it is just whitespace fixes. > > Ok, I see. In the given patch all the fixes are mainly auto-generated > and remove the trailing spaces. But I see your point. > > Thanks, > Artem. >> -- >> Joseph S. Myers >> joseph@codesourcery.com >> > Successfully regtested on x86-unknown-linux-gnu. Committed to the mainline with the revision 179588. ChangeLog: 2011-10-06 Artjoms Sinkarovs <artyom.shinkaroff@gmail.com> * c-tree.h (c_expr_t): New typedef for struct c_expr. (C_EXPR_APPEND): New macro. * c-parser.c (c_parser_get_builtin_args): Preserve original_tree_code of c_expr structure. Fixes bconstp-3.c failure of PR50607. (c_parser_postfix_expression): Adjust to the new function. Artem.
On Thu, 6 Oct 2011, Artem Shinkarov wrote: > Successfully regtested on x86-unknown-linux-gnu. Committed to the > mainline with the revision 179588. > > ChangeLog: > 2011-10-06 Artjoms Sinkarovs <artyom.shinkaroff@gmail.com> > * c-tree.h (c_expr_t): New typedef for struct c_expr. > (C_EXPR_APPEND): New macro. > * c-parser.c (c_parser_get_builtin_args): Preserve > original_tree_code of c_expr structure. Fixes bconstp-3.c > failure of PR50607. > (c_parser_postfix_expression): Adjust to the new function. Write that changelog entry as: PR middle-end/50607 * c-tree.h (c_expr_t): New typedef for struct c_expr. (C_EXPR_APPEND): New macro. * c-parser.c (c_parser_get_builtin_args): Preserve original_tree_code of c_expr structure. (c_parser_postfix_expression): Adjust to the new function. (note top marker and without the "Fixes...") and there'll be an automatic entry in bugzilla, like for example PR50609. You still have to close the bugzilla entry manually (not all noteworthy fixes are reason to close a bug report). Mostly as a note for the future but you could fix that now, when adding the missing empty line after the date+email line. ;) brgds, H-P
On Thu, Oct 6, 2011 at 3:27 AM, Hans-Peter Nilsson <hp@bitrange.com> wrote: > On Thu, 6 Oct 2011, Artem Shinkarov wrote: >> Successfully regtested on x86-unknown-linux-gnu. Committed to the >> mainline with the revision 179588. >> >> ChangeLog: >> 2011-10-06 Artjoms Sinkarovs <artyom.shinkaroff@gmail.com> >> * c-tree.h (c_expr_t): New typedef for struct c_expr. >> (C_EXPR_APPEND): New macro. >> * c-parser.c (c_parser_get_builtin_args): Preserve >> original_tree_code of c_expr structure. Fixes bconstp-3.c >> failure of PR50607. >> (c_parser_postfix_expression): Adjust to the new function. > > Write that changelog entry as: > > PR middle-end/50607 > * c-tree.h (c_expr_t): New typedef for struct c_expr. > (C_EXPR_APPEND): New macro. > * c-parser.c (c_parser_get_builtin_args): Preserve > original_tree_code of c_expr structure. > (c_parser_postfix_expression): Adjust to the new function. > > (note top marker and without the "Fixes...") > and there'll be an automatic entry in bugzilla, like for example > PR50609. You still have to close the bugzilla entry manually > (not all noteworthy fixes are reason to close a bug report). > > Mostly as a note for the future but you could fix that now, when > adding the missing empty line after the date+email line. ;) > > brgds, H-P > Thanks for the advice, I didn't know about the automatic mail. Fixed with commit 179589. As for closing bugzilla entry, I would prefer to make sure that the patch really fixes the problem. It would be very helpful if you could confirm this. Thanks, Artem.
On Thu, 6 Oct 2011, Artem Shinkarov wrote: > On Thu, Oct 6, 2011 at 3:27 AM, Hans-Peter Nilsson <hp@bitrange.com> wrote: > > Write that changelog entry as: > > > > PR middle-end/50607 > > * c-tree.h (c_expr_t): New typedef for struct c_expr. > > (C_EXPR_APPEND): New macro. > > * c-parser.c (c_parser_get_builtin_args): Preserve > > original_tree_code of c_expr structure. > > (c_parser_postfix_expression): Adjust to the new function. > > > > (note top marker and without the "Fixes...") > > and there'll be an automatic entry in bugzilla, like for example > > PR50609. You still have to close the bugzilla entry manually > > (not all noteworthy fixes are reason to close a bug report). > Thanks for the advice, I didn't know about the automatic mail. Correction: for the automatic-bugzilla-comment machinery to work, you have to mention it in the svn *checkin message* but it is advised anyway to have this equivalent to the body of the changelog entry. (In emacs, mark the ChangeLog text and do M-| "svn ci -F /dev/stdin ChangeLog files-that-were-modified".) > As for closing bugzilla entry, I would prefer to > make sure that the patch really fixes the problem. It would be very > helpful if you could confirm this. Sorry, I don't have an autotester result ready to confirm. (But thanks for asking, I had disabled my cris-elf autotester and forgot to re-enable it!) It is sufficient that you've done this yourself; you need to use your @gcc.gnu.org bugzilla account. Don't worry, you'll be reminded if the bug is closed prematurely. :) brgds, H-P
diff -up '-F^(define' gcc-bootstrap/gcc//c-parser.c gcc-new/gcc//c-parser.c --- gcc-bootstrap/gcc//c-parser.c 2011-10-05 00:09:59.067560839 +0100 +++ gcc-new/gcc//c-parser.c 2011-10-05 00:08:23.454756162 +0100 @@ -5993,16 +5993,16 @@ c_parser_alignof_expression (c_parser *p for the middle-end nodes like COMPLEX_EXPR, VEC_SHUFFLE_EXPR and others. The name of the builtin is passed using BNAME parameter. Function returns true if there were no errors while parsing and - stores the arguments in EXPR_LIST. List of original types can be - obtained by passing non NULL value to ORIG_TYPES. */ + stores the arguments in CEXPR_LIST. */ static bool c_parser_get_builtin_args (c_parser *parser, const char *bname, - VEC(tree,gc) **expr_list, - VEC(tree,gc) **orig_types) + VEC(c_expr_t,gc) **ret_cexpr_list) { location_t loc = c_parser_peek_token (parser)->location; - *expr_list = NULL; + VEC (c_expr_t,gc) *cexpr_list; + c_expr_t expr; + *ret_cexpr_list = NULL; if (c_parser_next_token_is_not (parser, CPP_OPEN_PAREN)) { error_at (loc, "cannot take address of %qs", bname); @@ -6017,14 +6017,20 @@ c_parser_get_builtin_args (c_parser *par return true; } - if (orig_types) - *expr_list = c_parser_expr_list (parser, false, false, orig_types); - else - *expr_list = c_parser_expr_list (parser, false, false, NULL); + expr = c_parser_expr_no_commas (parser, NULL); + cexpr_list = VEC_alloc (c_expr_t, gc, 1); + C_EXPR_APPEND (cexpr_list, expr); + while (c_parser_next_token_is (parser, CPP_COMMA)) + { + c_parser_consume_token (parser); + expr = c_parser_expr_no_commas (parser, NULL); + C_EXPR_APPEND (cexpr_list, expr); + } if (!c_parser_require (parser, CPP_CLOSE_PAREN, "expected %<)%>")) return false; + *ret_cexpr_list = cexpr_list; return true; } @@ -6378,20 +6384,20 @@ c_parser_postfix_expression (c_parser *p break; case RID_CHOOSE_EXPR: { - VEC(tree,gc) *expr_list; - VEC(tree,gc) *orig_types; - tree e1value, e2value, e3value, c; + VEC (c_expr_t, gc) *cexpr_list; + c_expr_t *e1_p, *e2_p, *e3_p; + tree c; c_parser_consume_token (parser); if (!c_parser_get_builtin_args (parser, "__builtin_choose_expr", - &expr_list, &orig_types)) + &cexpr_list)) { expr.value = error_mark_node; break; } - if (VEC_length (tree, expr_list) != 3) + if (VEC_length (c_expr_t, cexpr_list) != 3) { error_at (loc, "wrong number of arguments to " "%<__builtin_choose_expr%>"); @@ -6399,31 +6405,20 @@ c_parser_postfix_expression (c_parser *p break; } - e1value = VEC_index (tree, expr_list, 0); - e2value = VEC_index (tree, expr_list, 1); - e3value = VEC_index (tree, expr_list, 2); - - c = e1value; - mark_exp_read (e2value); - mark_exp_read (e3value); + e1_p = VEC_index (c_expr_t, cexpr_list, 0); + e2_p = VEC_index (c_expr_t, cexpr_list, 1); + e3_p = VEC_index (c_expr_t, cexpr_list, 2); + + c = e1_p->value; + mark_exp_read (e2_p->value); + mark_exp_read (e3_p->value); if (TREE_CODE (c) != INTEGER_CST || !INTEGRAL_TYPE_P (TREE_TYPE (c))) error_at (loc, "first argument to %<__builtin_choose_expr%> not" " a constant"); constant_expression_warning (c); - - if (integer_zerop (c)) - { - expr.value = e3value; - expr.original_type = VEC_index (tree, orig_types, 2); - } - else - { - expr.value = e2value; - expr.original_type = VEC_index (tree, orig_types, 1); - } - + expr = integer_zerop (c) ? *e3_p : *e2_p; break; } case RID_TYPES_COMPATIBLE_P: @@ -6465,19 +6460,19 @@ c_parser_postfix_expression (c_parser *p break; case RID_BUILTIN_COMPLEX: { - VEC(tree,gc) *expr_list; - tree e1value, e2value; + VEC(c_expr_t, gc) *cexpr_list; + c_expr_t *e1_p, *e2_p; c_parser_consume_token (parser); if (!c_parser_get_builtin_args (parser, "__builtin_complex", - &expr_list, NULL)) + &cexpr_list)) { expr.value = error_mark_node; break; } - if (VEC_length (tree, expr_list) != 2) + if (VEC_length (c_expr_t, cexpr_list) != 2) { error_at (loc, "wrong number of arguments to " "%<__builtin_complex%>"); @@ -6485,29 +6480,29 @@ c_parser_postfix_expression (c_parser *p break; } - e1value = VEC_index (tree, expr_list, 0); - e2value = VEC_index (tree, expr_list, 1); + e1_p = VEC_index (c_expr_t, cexpr_list, 0); + e2_p = VEC_index (c_expr_t, cexpr_list, 1); - mark_exp_read (e1value); - if (TREE_CODE (e1value) == EXCESS_PRECISION_EXPR) - e1value = convert (TREE_TYPE (e1value), - TREE_OPERAND (e1value, 0)); - mark_exp_read (e2value); - if (TREE_CODE (e2value) == EXCESS_PRECISION_EXPR) - e2value = convert (TREE_TYPE (e2value), - TREE_OPERAND (e2value, 0)); - if (!SCALAR_FLOAT_TYPE_P (TREE_TYPE (e1value)) - || DECIMAL_FLOAT_TYPE_P (TREE_TYPE (e1value)) - || !SCALAR_FLOAT_TYPE_P (TREE_TYPE (e2value)) - || DECIMAL_FLOAT_TYPE_P (TREE_TYPE (e2value))) + mark_exp_read (e1_p->value); + if (TREE_CODE (e1_p->value) == EXCESS_PRECISION_EXPR) + e1_p->value = convert (TREE_TYPE (e1_p->value), + TREE_OPERAND (e1_p->value, 0)); + mark_exp_read (e2_p->value); + if (TREE_CODE (e2_p->value) == EXCESS_PRECISION_EXPR) + e2_p->value = convert (TREE_TYPE (e2_p->value), + TREE_OPERAND (e2_p->value, 0)); + if (!SCALAR_FLOAT_TYPE_P (TREE_TYPE (e1_p->value)) + || DECIMAL_FLOAT_TYPE_P (TREE_TYPE (e1_p->value)) + || !SCALAR_FLOAT_TYPE_P (TREE_TYPE (e2_p->value)) + || DECIMAL_FLOAT_TYPE_P (TREE_TYPE (e2_p->value))) { error_at (loc, "%<__builtin_complex%> operand " "not of real binary floating-point type"); expr.value = error_mark_node; break; } - if (TYPE_MAIN_VARIANT (TREE_TYPE (e1value)) - != TYPE_MAIN_VARIANT (TREE_TYPE (e2value))) + if (TYPE_MAIN_VARIANT (TREE_TYPE (e1_p->value)) + != TYPE_MAIN_VARIANT (TREE_TYPE (e2_p->value))) { error_at (loc, "%<__builtin_complex%> operands of different types"); @@ -6518,34 +6513,37 @@ c_parser_postfix_expression (c_parser *p pedwarn (loc, OPT_pedantic, "ISO C90 does not support complex types"); expr.value = build2 (COMPLEX_EXPR, - build_complex_type (TYPE_MAIN_VARIANT - (TREE_TYPE (e1value))), - e1value, e2value); + build_complex_type + (TYPE_MAIN_VARIANT + (TREE_TYPE (e1_p->value))), + e1_p->value, e2_p->value); break; } case RID_BUILTIN_SHUFFLE: { - VEC(tree,gc) *expr_list; + VEC(c_expr_t,gc) *cexpr_list; c_parser_consume_token (parser); if (!c_parser_get_builtin_args (parser, "__builtin_shuffle", - &expr_list, NULL)) + &cexpr_list)) { expr.value = error_mark_node; break; } - if (VEC_length (tree, expr_list) == 2) - expr.value = c_build_vec_shuffle_expr - (loc, VEC_index (tree, expr_list, 0), - NULL_TREE, - VEC_index (tree, expr_list, 1)); - else if (VEC_length (tree, expr_list) == 3) - expr.value = c_build_vec_shuffle_expr - (loc, VEC_index (tree, expr_list, 0), - VEC_index (tree, expr_list, 1), - VEC_index (tree, expr_list, 2)); + if (VEC_length (c_expr_t, cexpr_list) == 2) + expr.value = + c_build_vec_shuffle_expr + (loc, VEC_index (c_expr_t, cexpr_list, 0)->value, + NULL_TREE, VEC_index (c_expr_t, cexpr_list, 1)->value); + + else if (VEC_length (c_expr_t, cexpr_list) == 3) + expr.value = + c_build_vec_shuffle_expr + (loc, VEC_index (c_expr_t, cexpr_list, 0)->value, + VEC_index (c_expr_t, cexpr_list, 1)->value, + VEC_index (c_expr_t, cexpr_list, 2)->value); else { error_at (loc, "wrong number of arguments to " diff -up '-F^(define' gcc-bootstrap/gcc//c-tree.h gcc-new/gcc//c-tree.h --- gcc-bootstrap/gcc//c-tree.h 2011-10-05 00:10:06.419468926 +0100 +++ gcc-new/gcc//c-tree.h 2011-10-05 00:08:23.475755901 +0100 @@ -130,6 +130,22 @@ struct c_expr tree original_type; }; +/* Type alias for struct c_expr. This allows to use the structure + inside the VEC types. */ +typedef struct c_expr c_expr_t; + +/* A varray of c_expr_t. */ +DEF_VEC_O (c_expr_t); +DEF_VEC_ALLOC_O (c_expr_t, gc); +DEF_VEC_ALLOC_O (c_expr_t, heap); + +/* Append a new c_expr_t element to V. */ +#define C_EXPR_APPEND(V, ELEM) \ + do { \ + c_expr_t *__elem_p = VEC_safe_push (c_expr_t, gc, V, NULL); \ + *__elem_p = (ELEM); \ + } while (0) + /* A kind of type specifier. Note that this information is currently only used to distinguish tag definitions, tag references and typeof uses. */