On Mon, 2017-08-28 at 09:22 -0600, Jeff Law wrote: > On 07/03/2017 12:37 PM, David Malcolm wrote: > > This patch improves our C/C++ frontends' handling of missing > > symbols, by making c_parser_require and cp_parser_require use > > "better" locations for the diagnostic, and insert fix-it hints, > > under certain circumstances (see the comments in the patch for > > full details). > > > > For example, for this code with a missing semicolon: > > > > $ cat test.c > > int missing_semicolon (void) > > { > > return 42 > > } > > > > trunk currently emits: > > > > test.c:4:1: error: expected ‘;’ before ‘}’ token > > } > > ^ > > > > This patch adds a fix-it hint for the missing semicolon, and puts > > the error at the location of the missing semicolon, printing the > > followup token as a secondary location: > > > > test.c:3:12: error: expected ‘;’ before ‘}’ token > > return 42 > > ^ > > ; > > } > > ~ > > > > More examples can be seen in the test cases. > > > > For reference, clang prints the following: > > > > test.c:3:12: error: expected ';' after return statement > > return 42 > > ^ > > ; > > > > i.e. describing what syntactic thing came before, which > > I think is likely to be more meaningful to the user. > > > > clang can also print notes about matching opening symbols > > e.g. the note here: > > > > missing-symbol-2.c:25:22: error: expected ']' > > const char test [42; > > ^ > > missing-symbol-2.c:25:19: note: to match this '[' > > const char test [42; > > ^ > > which, although somewhat redundant for this example, seems much > > more > > useful if there's non-trivial nesting of constructs, or more than a > > few > > lines separating the open/close symbols (e.g. showing a stray > > "namespace {" > > that the user forgot to close). > > > > I'd like to implement both of these ideas as followups, but in > > the meantime, is the fix-it hint patch OK for trunk? > > (successfully bootstrapped & regrtested on x86_64-pc-linux-gnu) > > > > gcc/c-family/ChangeLog: > > * c-common.c (c_parse_error): Add RICHLOC param, and use it > > rather > > than implicitly using input_location. > > (enum missing_token_insertion_kind): New enum. > > (get_missing_token_insertion_kind): New function. > > (maybe_suggest_missing_token_insertion): New function. > > * c-common.h (c_parse_error): Add RICHLOC param. > > (maybe_suggest_missing_token_insertion): New decl. > > > > gcc/c/ChangeLog: > > * c-parser.c (struct c_parser): Add "previous_token_loc" field. > > (c_parser_consume_token): Set parser->previous_token_loc. > > (c_parser_error): Rename to... > > (c_parser_error_richloc): ...this, making static, and adding > > "richloc" parameter, passing it to the c_parse_error call, > > rather than calling c_parser_set_source_position_from_token. > > (c_parser_error): Reintroduce, reimplementing in terms of the > > above. > > (c_parser_require): Add "type_is_unique" param. Use > > c_parser_error_richloc rather than c_parser_error, calling > > maybe_suggest_missing_token_insertion. > > (c_parser_parms_list_declarator): Override default value of new > > "type_is_unique" param to c_parser_require. > > (c_parser_asm_statement): Likewise. > > * c-parser.h (c_parser_require): Add "type_is_unique" param, > > defaulting to true. > > > > gcc/cp/ChangeLog: > > * parser.c (cp_parser_error): Add rich_location to call to > > c_parse_error. > > (get_required_cpp_ttype): New function. > > (cp_parser_required_error): Remove calls to cp_parser_error, > > instead setting a non-NULL gmsgid, and handling it if set by > > calling c_parse_error, potentially with a fix-it hint. > > > > gcc/testsuite/ChangeLog: > > * c-c++-common/cilk-plus/AN/parser_errors.c: Update expected > > output to reflect changes to reported locations of missing > > symbols. > > * c-c++-common/cilk-plus/AN/parser_errors2.c: Likewise. > > * c-c++-common/cilk-plus/AN/parser_errors3.c: Likewise. > > * c-c++-common/cilk-plus/AN/pr61191.c: Likewise. > > * c-c++-common/gomp/pr63326.c: Likewise. > > * c-c++-common/missing-symbol.c: New test case. > > * g++.dg/cpp1y/digit-sep-neg.C: Update expected output to > > reflect > > changes to reported locations of missing symbols. > > * g++.dg/cpp1y/pr65202.C: Likewise. > > * g++.dg/other/do1.C: Likewise. > > * g++.dg/missing-symbol-2.C: New test case. > > * g++.dg/parse/error11.C: Update expected output to reflect > > changes to reported locations of missing symbols. > > * g++.dg/parse/pragma2.C: Likewise. > > * g++.dg/template/error11.C: Likewise. > > * gcc.dg/missing-symbol-2.c: New test case. > > * gcc.dg/missing-symbol-3.c: New test case. > > * gcc.dg/noncompile/940112-1.c: Update expected output to > > reflect > > changes to reported locations of missing symbols. > > * gcc.dg/noncompile/971104-1.c: Likewise. > > * obj-c++.dg/exceptions-6.mm: Likewise. > > * obj-c++.dg/pr48187.mm: Likewise. > > * objc.dg/exceptions-6.m: Likewise. > > AFAICT, this never got moved forward after the comments from Richard > and > Joseph.
Here's an updated version of the patch, addressing the various issues raised. > > +} > > + > > +/* Given RICHLOC, a location for a diagnostic describing a missing > > token > > + of kind TOKEN_TYPE, potentially add a fix-it hint suggesting > > the > > + insertion of the token. > > + > > + The location of the attemped fix-it hint depends on TOKEN_TYPE: > > s/attemped/attempted/ Fixed. > > + > > + if (gmsgid) > > + { > > + /* Emulate rest of cp_parser_error. */ > > + cp_token *token = cp_lexer_peek_token (parser->lexer); > > + cp_lexer_set_source_position_from_token (token); > > + > > + rich_location richloc (line_table, input_location); > > So is it worth trying to factor the bits you want to emulate from > cp_parser_error so that they're shared? Or just a comment in > cp_parser_error in the hopes that if someone changes it in a > meaningful > way they'll know to come back here and potentially update this > routine? I ended up factoring out the shared bits into a new subroutine (patch 1 of the kit). > In general though it looks really good. I think we just want to make > a > decision whether or not there's some way to avoid a long term > maintenance headache noted immediately above. > > jeff > Jeff Thanks. Here's an updated version of the patch, split out as a two patch kit. Patch 1 implements the cleanup of the duplicated code. Patch 2 implements the fix-it hints. Are they OK for trunk? Dave David Malcolm (2): C++: avoid partial duplicate implementation of cp_parser_error C/C++: add fix-it hints for various missing symbols (v2) gcc/c-family/c-common.c | 158 +++++++++++++++ gcc/c-family/c-common.h | 3 + gcc/c/c-parser.c | 29 ++- gcc/c/c-parser.h | 3 +- gcc/cp/parser.c | 214 ++++++++++++++------- .../c-c++-common/cilk-plus/AN/parser_errors.c | 4 +- .../c-c++-common/cilk-plus/AN/parser_errors2.c | 3 +- .../c-c++-common/cilk-plus/AN/parser_errors3.c | 3 +- gcc/testsuite/c-c++-common/cilk-plus/AN/pr61191.c | 3 +- gcc/testsuite/c-c++-common/gomp/pr63326.c | 22 +-- gcc/testsuite/c-c++-common/missing-close-symbol.c | 2 + gcc/testsuite/c-c++-common/missing-symbol.c | 35 ++-- gcc/testsuite/g++.dg/cpp1y/digit-sep-neg.C | 4 +- gcc/testsuite/g++.dg/cpp1y/pr65202.C | 4 +- gcc/testsuite/g++.dg/missing-symbol-2.C | 58 ++++++ gcc/testsuite/g++.dg/other/do1.C | 4 +- gcc/testsuite/g++.dg/parse/error11.C | 2 +- gcc/testsuite/g++.dg/parse/pragma2.C | 4 +- gcc/testsuite/g++.dg/template/error11.C | 2 +- gcc/testsuite/gcc.dg/missing-symbol-2.c | 71 +++++++ gcc/testsuite/gcc.dg/missing-symbol-3.c | 50 +++++ gcc/testsuite/gcc.dg/noncompile/940112-1.c | 4 +- gcc/testsuite/gcc.dg/noncompile/971104-1.c | 4 +- gcc/testsuite/obj-c++.dg/exceptions-6.mm | 6 +- gcc/testsuite/obj-c++.dg/pr48187.mm | 8 +- gcc/testsuite/objc.dg/exceptions-6.m | 4 +- 26 files changed, 574 insertions(+), 130 deletions(-) create mode 100644 gcc/testsuite/g++.dg/missing-symbol-2.C create mode 100644 gcc/testsuite/gcc.dg/missing-symbol-2.c create mode 100644 gcc/testsuite/gcc.dg/missing-symbol-3.c -- 1.8.5.3