| Message ID | CAJhB01mDQNQCmtmsVSiaDduQTSMP2NgBfrmQA2TYuXnqjQw_TA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] Improve bad error message with stray semicolon in initializer (and related) [PR101232] | expand |
On 05/08/24 09:32 +0200, Franciszek Witt wrote: >Hi, could someone review this patch? > >This is built on top of the v2 from >https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101232 the only >difference is fix for error59.C > >I have tested it on x86_64 Ubuntu 22 machine. > > >Regards >Franciszek > >--- > >Author: Franciszek Witt <franek.witt@gmail.com> >Date: Mon Aug 5 09:00:35 2024 +0200 > > c++: [PR101232] The line above should be the first line of your Git commit message, which should be in the form "component: Summary of change [PRnnnn]" The Subject: of your email seems like it should be repeated here (although something shorter would be even nicer, if possible). https://cbea.ms/git-commit/ > PR c++/101232 > > gcc/cp/ChangeLog: > > * parser.cc (cp_parser_postfix_expression): Commit to the >parse in case we know its either a cast or invalid syntax. > (cp_parser_braced_list): Add a heuristic to inform about >missing comma or operator. The ChangeLog part of the commit message should follow the rules in the GNU Coding Standards: https://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html Specifically, they should be wrapped to fit in an 80 column terminal window. I think wrapping around 76 columns is common. Please add line breaks here, e.g. * parser.cc (cp_parser_postfix_expression): Commit to the parse in case we know its either a cast or invalid syntax. (cp_parser_braced_list): Add a heuristic to inform about missing comma or operator. (but with a leading TAB, not spaces).
On 8/19/24 6:01 AM, Jonathan Wakely wrote: > On 05/08/24 09:32 +0200, Franciszek Witt wrote: >> Author: Franciszek Witt <franek.witt@gmail.com> >> Date: Mon Aug 5 09:00:35 2024 +0200 >> >> c++: [PR101232] > > The line above should be the first line of your Git commit message, > which should be in the form "component: Summary of change [PRnnnn]" > The Subject: of your email seems like it should be repeated here > (although something shorter would be even nicer, if possible). > https://cbea.ms/git-commit/ > > >> PR c++/101232 >> >> gcc/cp/ChangeLog: >> >> * parser.cc (cp_parser_postfix_expression): Commit to the >> parse in case we know its either a cast or invalid syntax. >> (cp_parser_braced_list): Add a heuristic to inform about >> missing comma or operator. > > The ChangeLog part of the commit message should follow the rules in > the GNU Coding Standards: > https://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html > > Specifically, they should be wrapped to fit in an 80 column terminal > window. I think wrapping around 76 columns is common. Please add line > breaks here, e.g. > > * parser.cc (cp_parser_postfix_expression): Commit to the > parse in case we know its either a cast or invalid syntax. > (cp_parser_braced_list): Add a heuristic to inform about > missing comma or operator. > > (but with a leading TAB, not spaces). Agreed. In addition, we need DCO certification or copyright assignment (https://gcc.gnu.org/contribute.html#legal). Also, the patch itself was corrupted by word wrap in your mail client, so it doesn't apply, e.g.: > @@ -7881,8 +7881,13 @@ cp_parser_postfix_expression (cp_parser > *parser, bool address_p, bool cast_p, https://www.kernel.org/doc/html/latest/process/email-clients.html has some tips for avoiding word wrap, or you can just attach the patch instead of pasting it inline. > + /* This is just a heuristic. */ Two spaces after the . > + inform (finish_loc, > + "probably missing a comma or an operator before"); The string should line up after the ( on the previous line. We might as well also put this inform under the cp_parser_skip_to_closing_brace check; if there's no } at all, the problem isn't a missing comma... Jason
On 8/19/24 12:01 PM, Jason Merrill wrote: > On 8/19/24 6:01 AM, Jonathan Wakely wrote: >> On 05/08/24 09:32 +0200, Franciszek Witt wrote: >>> Author: Franciszek Witt <franek.witt@gmail.com> >>> Date: Mon Aug 5 09:00:35 2024 +0200 >>> >>> c++: [PR101232] >> >> The line above should be the first line of your Git commit message, >> which should be in the form "component: Summary of change [PRnnnn]" >> The Subject: of your email seems like it should be repeated here >> (although something shorter would be even nicer, if possible). >> https://cbea.ms/git-commit/ >> >> >>> PR c++/101232 >>> >>> gcc/cp/ChangeLog: >>> >>> * parser.cc (cp_parser_postfix_expression): Commit to the >>> parse in case we know its either a cast or invalid syntax. >>> (cp_parser_braced_list): Add a heuristic to inform about >>> missing comma or operator. >> >> The ChangeLog part of the commit message should follow the rules in >> the GNU Coding Standards: >> https://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html >> >> Specifically, they should be wrapped to fit in an 80 column terminal >> window. I think wrapping around 76 columns is common. Please add line >> breaks here, e.g. >> >> * parser.cc (cp_parser_postfix_expression): Commit to the >> parse in case we know its either a cast or invalid syntax. >> (cp_parser_braced_list): Add a heuristic to inform about >> missing comma or operator. >> >> (but with a leading TAB, not spaces). > > Agreed. In addition, we need DCO certification or copyright assignment > (https://gcc.gnu.org/contribute.html#legal). > > Also, the patch itself was corrupted by word wrap in your mail client, > so it doesn't apply, e.g.: > >> @@ -7881,8 +7881,13 @@ cp_parser_postfix_expression (cp_parser >> *parser, bool address_p, bool cast_p, > > https://www.kernel.org/doc/html/latest/process/email-clients.html has > some tips for avoiding word wrap, or you can just attach the patch > instead of pasting it inline. > >> + /* This is just a heuristic. */ > > Two spaces after the . > >> + inform (finish_loc, + "probably missing a comma >> or an operator before"); > > The string should line up after the ( on the previous line. > > We might as well also put this inform under the > cp_parser_skip_to_closing_brace check; if there's no } at all, the > problem isn't a missing comma... Also, error59 seems like an error-recovery regression: > +++ b/gcc/testsuite/g++.dg/parse/error59.C > @@ -3,4 +3,4 @@ > void foo() > { > (struct {}x){}; // { dg-error "" } > -} > +} // { dg-excess-errors "" } as we now get more of an error cascade. Perhaps also check the return value of the earlier require_open and don't skip_to_closing_brace if it failed? Jason
Thanks for the feedback. I am attatching fixed patch as a file. Regards Franciszek On Mon, 19 Aug 2024, 18:32 Jason Merrill, <jason@redhat.com> wrote: > On 8/19/24 12:01 PM, Jason Merrill wrote: > > On 8/19/24 6:01 AM, Jonathan Wakely wrote: > >> On 05/08/24 09:32 +0200, Franciszek Witt wrote: > >>> Author: Franciszek Witt <franek.witt@gmail.com> > >>> Date: Mon Aug 5 09:00:35 2024 +0200 > >>> > >>> c++: [PR101232] > >> > >> The line above should be the first line of your Git commit message, > >> which should be in the form "component: Summary of change [PRnnnn]" > >> The Subject: of your email seems like it should be repeated here > >> (although something shorter would be even nicer, if possible). > >> https://cbea.ms/git-commit/ > >> > >> > >>> PR c++/101232 > >>> > >>> gcc/cp/ChangeLog: > >>> > >>> * parser.cc (cp_parser_postfix_expression): Commit to the > >>> parse in case we know its either a cast or invalid syntax. > >>> (cp_parser_braced_list): Add a heuristic to inform about > >>> missing comma or operator. > >> > >> The ChangeLog part of the commit message should follow the rules in > >> the GNU Coding Standards: > >> https://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html > >> > >> Specifically, they should be wrapped to fit in an 80 column terminal > >> window. I think wrapping around 76 columns is common. Please add line > >> breaks here, e.g. > >> > >> * parser.cc (cp_parser_postfix_expression): Commit to the > >> parse in case we know its either a cast or invalid syntax. > >> (cp_parser_braced_list): Add a heuristic to inform about > >> missing comma or operator. > >> > >> (but with a leading TAB, not spaces). > > > > Agreed. In addition, we need DCO certification or copyright assignment > > (https://gcc.gnu.org/contribute.html#legal). > > > > Also, the patch itself was corrupted by word wrap in your mail client, > > so it doesn't apply, e.g.: > > > >> @@ -7881,8 +7881,13 @@ cp_parser_postfix_expression (cp_parser > >> *parser, bool address_p, bool cast_p, > > > > https://www.kernel.org/doc/html/latest/process/email-clients.html has > > some tips for avoiding word wrap, or you can just attach the patch > > instead of pasting it inline. > > > >> + /* This is just a heuristic. */ > > > > Two spaces after the . > > > >> + inform (finish_loc, + "probably missing a comma > >> or an operator before"); > > > > The string should line up after the ( on the previous line. > > > > We might as well also put this inform under the > > cp_parser_skip_to_closing_brace check; if there's no } at all, the > > problem isn't a missing comma... > > Also, error59 seems like an error-recovery regression: > > > +++ b/gcc/testsuite/g++.dg/parse/error59.C > > @@ -3,4 +3,4 @@ > > void foo() > > { > > (struct {}x){}; // { dg-error "" } > > -} > > +} // { dg-excess-errors "" } > > as we now get more of an error cascade. Perhaps also check the return > value of the earlier require_open and don't skip_to_closing_brace if it > failed? > > Jason > >
On 8/20/24 9:11 AM, Franciszek Witt wrote: > Thanks for the feedback. I am attatching fixed patch as a file. Pushed, thanks. > Regards > Franciszek > > On Mon, 19 Aug 2024, 18:32 Jason Merrill, <jason@redhat.com > <mailto:jason@redhat.com>> wrote: > > On 8/19/24 12:01 PM, Jason Merrill wrote: > > On 8/19/24 6:01 AM, Jonathan Wakely wrote: > >> On 05/08/24 09:32 +0200, Franciszek Witt wrote: > >>> Author: Franciszek Witt <franek.witt@gmail.com > <mailto:franek.witt@gmail.com>> > >>> Date: Mon Aug 5 09:00:35 2024 +0200 > >>> > >>> c++: [PR101232] > >> > >> The line above should be the first line of your Git commit message, > >> which should be in the form "component: Summary of change [PRnnnn]" > >> The Subject: of your email seems like it should be repeated here > >> (although something shorter would be even nicer, if possible). > >> https://cbea.ms/git-commit/ <https://cbea.ms/git-commit/> > >> > >> > >>> PR c++/101232 > >>> > >>> gcc/cp/ChangeLog: > >>> > >>> * parser.cc (cp_parser_postfix_expression): Commit > to the > >>> parse in case we know its either a cast or invalid syntax. > >>> (cp_parser_braced_list): Add a heuristic to inform about > >>> missing comma or operator. > >> > >> The ChangeLog part of the commit message should follow the rules in > >> the GNU Coding Standards: > >> > https://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html <https://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html> > >> > >> Specifically, they should be wrapped to fit in an 80 column terminal > >> window. I think wrapping around 76 columns is common. Please add > line > >> breaks here, e.g. > >> > >> * parser.cc (cp_parser_postfix_expression): Commit to the > >> parse in case we know its either a cast or invalid syntax. > >> (cp_parser_braced_list): Add a heuristic to inform about > >> missing comma or operator. > >> > >> (but with a leading TAB, not spaces). > > > > Agreed. In addition, we need DCO certification or copyright > assignment > > (https://gcc.gnu.org/contribute.html#legal > <https://gcc.gnu.org/contribute.html#legal>). > > > > Also, the patch itself was corrupted by word wrap in your mail > client, > > so it doesn't apply, e.g.: > > > >> @@ -7881,8 +7881,13 @@ cp_parser_postfix_expression (cp_parser > >> *parser, bool address_p, bool cast_p, > > > > https://www.kernel.org/doc/html/latest/process/email-clients.html > <https://www.kernel.org/doc/html/latest/process/email-clients.html> has > > some tips for avoiding word wrap, or you can just attach the patch > > instead of pasting it inline. > > > >> + /* This is just a heuristic. */ > > > > Two spaces after the . > > > >> + inform (finish_loc, + "probably missing a > comma > >> or an operator before"); > > > > The string should line up after the ( on the previous line. > > > > We might as well also put this inform under the > > cp_parser_skip_to_closing_brace check; if there's no } at all, the > > problem isn't a missing comma... > > Also, error59 seems like an error-recovery regression: > > > +++ b/gcc/testsuite/g++.dg/parse/error59.C > > @@ -3,4 +3,4 @@ > > void foo() > > { > > (struct {}x){}; // { dg-error "" } > > -} > > +} // { dg-excess-errors "" } > > as we now get more of an error cascade. Perhaps also check the return > value of the earlier require_open and don't skip_to_closing_brace if it > failed? > > Jason >
diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc index 1dd0efaf963..2e0ce1c6ddb 100644 --- a/gcc/cp/parser.cc +++ b/gcc/cp/parser.cc @@ -7881,8 +7881,13 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p, --parser->prevent_constrained_type_specifiers; /* Parse the cast itself. */ if (!cp_parser_error_occurred (parser)) - postfix_expression - = cp_parser_functional_cast (parser, type); + { + if (cp_lexer_next_token_is (parser->lexer, CPP_OPEN_BRACE)) + /* This can only be a cast. */ + cp_parser_commit_to_topmost_tentative_parse (parser); + postfix_expression + = cp_parser_functional_cast (parser, type); + } /* If that worked, we're done. */ if (cp_parser_parse_definitely (parser)) break; @@ -26350,8 +26355,19 @@ cp_parser_braced_list (cp_parser *parser, bool *non_constant_p /*=nullptr*/) else if (non_constant_p) *non_constant_p = false; /* Now, there should be a trailing `}'. */ - location_t finish_loc = cp_lexer_peek_token (parser->lexer)->location; - braces.require_close (parser); + cp_token * token = cp_lexer_peek_token (parser->lexer); + location_t finish_loc = token->location; + if (!braces.require_close (parser)) + { + /* This is just a heuristic. */ + if (token->type != CPP_SEMICOLON) + { + inform (finish_loc, + "probably missing a comma or an operator before"); + if (cp_parser_skip_to_closing_brace (parser)) + cp_lexer_consume_token (parser->lexer); + } + } TREE_TYPE (initializer) = init_list_type_node; recompute_constructor_flags (initializer); diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist-err1.C b/gcc/testsuite/g++.dg/cpp0x/initlist-err1.C new file mode 100644 index 00000000000..6ea8afb3273 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/initlist-err1.C @@ -0,0 +1,11 @@ +// PR c++/101232 +// { dg-do compile { target c++11 } } + +struct X { + int a; + int b; +}; + +void f() { + auto x = X{ 1, 2; }; // { dg-error "21:" } +} // { dg-prune-output "expected declaration" } diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist-err2.C b/gcc/testsuite/g++.dg/cpp0x/initlist-err2.C new file mode 100644 index 00000000000..227f519dc19 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/initlist-err2.C @@ -0,0 +1,11 @@ +// PR c++/101232 +// { dg-do compile { target c++11 } } + +struct X { + int a; + int b; +}; + +void f() { + auto x = X{ 1 2 }; // { dg-error "19:.*probably" } +} diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist-err3.C b/gcc/testsuite/g++.dg/cpp0x/initlist-err3.C new file mode 100644 index 00000000000..b77ec9bf4e9 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/initlist-err3.C @@ -0,0 +1,11 @@ +// PR c++/101232 +// { dg-do compile { target c++11 } } + +struct X { + int a; + int b; +}; + +void f() { + auto x = X{ 1, {2 }; // { dg-error "expected.*before" } +} diff --git a/gcc/testsuite/g++.dg/parse/error59.C b/gcc/testsuite/g++.dg/parse/error59.C index 2c44e210366..d782c9b1616 100644 --- a/gcc/testsuite/g++.dg/parse/error59.C +++ b/gcc/testsuite/g++.dg/parse/error59.C @@ -3,4 +3,4 @@ void foo() { (struct {}x){}; // { dg-error "" } -} +} // { dg-excess-errors "" }
Hi, could someone review this patch? This is built on top of the v2 from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101232 the only difference is fix for error59.C I have tested it on x86_64 Ubuntu 22 machine. Regards Franciszek --- Author: Franciszek Witt <franek.witt@gmail.com> Date: Mon Aug 5 09:00:35 2024 +0200 c++: [PR101232] PR c++/101232 gcc/cp/ChangeLog: * parser.cc (cp_parser_postfix_expression): Commit to the parse in case we know its either a cast or invalid syntax. (cp_parser_braced_list): Add a heuristic to inform about missing comma or operator. gcc/testsuite/ChangeLog: * g++.dg/parse/error59.C: Change the test so the new error message is accepted. * g++.dg/cpp0x/initlist-err1.C: New test. * g++.dg/cpp0x/initlist-err2.C: New test. * g++.dg/cpp0x/initlist-err3.C: New test. ---