| Message ID | 1461853698-43954-2-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/28/2016 04:28 PM, David Malcolm wrote: > whereas clang reportedly emits: > > test.c:2:12: error: expected ';' after struct > struct a {} > ^ > ; > > (note the offset of the location, and the fix-it hint) > > The following patch gives us the latter, more readable output. Huh. Only the non-C++ parts remain to be reviewed, and I have no technical objections, but do people really want this? To me that looks like unnecessary visual clutter that eats up vertical space for no reason. I know what a semicolon looks like without the compiler telling me twice. Bernd
On Mon, 2016-05-02 at 12:40 +0200, Bernd Schmidt wrote: > On 04/28/2016 04:28 PM, David Malcolm wrote: > > whereas clang reportedly emits: > > > > test.c:2:12: error: expected ';' after struct > > struct a {} > > ^ > > ; > > > > (note the offset of the location, and the fix-it hint) > > > > The following patch gives us the latter, more readable output. > > Huh. Only the non-C++ parts remain to be reviewed, and I have no > technical objections, but do people really want this? To me that > looks > like unnecessary visual clutter that eats up vertical space for no > reason. I know what a semicolon looks like without the compiler > telling > me twice. My own opinion is that it's worth spending the extra line to get the semicolon under the caret, as (IMHO) it makes things slightly clearer. A better argument is that as of r237712 we now have -fdiagnostics -parseable-fixits. This allows for an IDE to offer to automatically apply a fix-it hint. Hence by providing a fix-it here, an IDE can potentially insert the semicolon itself: $ ./xgcc -B. ../../src/gcc/testsuite/g++.dg/pr62314-2.C \ -fdiagnostics-parseable-fixits ../../src/gcc/testsuite/g++.dg/pr62314-2.C:4:11: error: expected ‘;’ after class definition class a {} // { dg-error "11: expected .;. after class definition" } ^ ; fix-it:"../../src/gcc/testsuite/g++.dg/pr62314-2.C":{4:11-4:11}:";" ../../src/gcc/testsuite/g++.dg/pr62314-2.C:8:2: error: expected ‘;’ after struct definition } // { dg-error "2: expected .;. after struct definition" } ^ ; fix-it:"../../src/gcc/testsuite/g++.dg/pr62314-2.C":{8:2-8:2}:";" I believe that on building this within a sufficiently recent version of Xcode that Xcode can offer to insert the semicolon directly. I'm hoping someone implements this for Emacs. In that light, is the patch OK? Thanks Dave
On 07/01/2016 07:40 PM, David Malcolm wrote: > > A better argument is that as of r237712 we now have -fdiagnostics > -parseable-fixits. This allows for an IDE to offer to automatically > apply a fix-it hint. Hence by providing a fix-it here, an IDE can > potentially insert the semicolon itself: > In that light, is the patch OK? Yeah, that's an argument I can buy. Bernd
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index ff16f73..e3133d0 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -21440,17 +21440,30 @@ cp_parser_class_specifier_1 (cp_parser* parser) closing brace. */ if (closing_brace && TYPE_P (type) && want_semicolon) { + /* Locate the closing brace. */ cp_token_position prev = cp_lexer_previous_token_position (parser->lexer); cp_token *prev_token = cp_lexer_token_at (parser->lexer, prev); location_t loc = prev_token->location; + /* We want to suggest insertion of a ';' immediately *after* the + closing brace, so, if we can, offset the location by 1 column. */ + location_t next_loc = loc; + if (!linemap_location_from_macro_expansion_p (line_table, loc)) + next_loc = linemap_position_for_loc_and_offset (line_table, loc, 1); + + rich_location richloc (line_table, next_loc); + richloc.add_fixit_insert (next_loc, ";"); + if (CLASSTYPE_DECLARED_CLASS (type)) - error_at (loc, "expected %<;%> after class definition"); + error_at_rich_loc (&richloc, + "expected %<;%> after class definition"); else if (TREE_CODE (type) == RECORD_TYPE) - error_at (loc, "expected %<;%> after struct definition"); + error_at_rich_loc (&richloc, + "expected %<;%> after struct definition"); else if (TREE_CODE (type) == UNION_TYPE) - error_at (loc, "expected %<;%> after union definition"); + error_at_rich_loc (&richloc, + "expected %<;%> after union definition"); else gcc_unreachable (); diff --git a/gcc/testsuite/g++.dg/parse/error5.C b/gcc/testsuite/g++.dg/parse/error5.C index eb1f9c7..d14a476 100644 --- a/gcc/testsuite/g++.dg/parse/error5.C +++ b/gcc/testsuite/g++.dg/parse/error5.C @@ -13,7 +13,7 @@ class Foo { int foo() return 0; } }; // need make cp_parser_error() report more accurate column numbers. // { dg-error "30:expected '\{' at end of input" "brace" { target *-*-* } 4 } -// { dg-error "33:expected ';' after class definition" "semicolon" {target *-*-* } 4 } +// { dg-error "34:expected ';' after class definition" "semicolon" {target *-*-* } 4 } // { dg-error "35:expected declaration before '\}' token" "declaration" {target *-*-* } 4 } diff --git a/gcc/testsuite/g++.dg/pr62314-2.C b/gcc/testsuite/g++.dg/pr62314-2.C new file mode 100644 index 0000000..deb0cb7 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr62314-2.C @@ -0,0 +1,22 @@ +// { dg-options "-fdiagnostics-show-caret" } + +template<class T> +class a {} // { dg-error "11: expected .;. after class definition" } +class temp {}; +a<temp> b; +struct b { +} // { dg-error "2: expected .;. after struct definition" } + +/* Verify that we emit fixit hints. */ + +/* { dg-begin-multiline-output "" } + class a {} + ^ + ; + { dg-end-multiline-output "" } */ + +/* { dg-begin-multiline-output "" } + } + ^ + ; + { dg-end-multiline-output "" } */
Looking over the discussion of missing semicolons in "Quality of Implementation and Attention to Detail" within http://clang.llvm.org/diagnostics.html and comparing with https://gcc.gnu.org/wiki/ClangDiagnosticsComparison I noticed that of the cases we do handle [1], there's room for improvement; we currently emit: test.c:2:11: error: expected ';' after struct definition struct a {} ^ whereas clang reportedly emits: test.c:2:12: error: expected ';' after struct struct a {} ^ ; (note the offset of the location, and the fix-it hint) The following patch gives us the latter, more readable output. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. OK for trunk? [1] I've also filed PR c++/68970 about a case given on the clang page that we still don't handle. gcc/cp/ChangeLog: PR c++/62314 * parser.c (cp_parser_class_specifier_1): When reporting missing semicolons, use a fixit-hint to suggest insertion of a semicolon immediately after the closing brace, offsetting the reported column accordingly. gcc/testsuite/ChangeLog: PR c++/62314 * gcc/testsuite/g++.dg/parse/error5.C: Update column number of missing semicolon error. * g++.dg/pr62314-2.C: New test case. --- gcc/cp/parser.c | 19 ++++++++++++++++--- gcc/testsuite/g++.dg/parse/error5.C | 2 +- gcc/testsuite/g++.dg/pr62314-2.C | 22 ++++++++++++++++++++++ 3 files changed, 39 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/g++.dg/pr62314-2.C