| Message ID | tkrat.bd65db21573d190b@netcologne.de |
|---|---|
| State | New |
| Headers | show |
On 04/29/2017 04:23 PM, Volker Reichelt wrote: > Hi, > > the following patch adds fix-it hints to the C++ parser for two wrongly > used keywords detected in cp_parser_decl_specifier_seq. > > Bootstrapped and regtested on x86_64-pc-linux-gnu. > > OK for trunk? > > Regards, > Volker > > > 2017-04-29 Volker Reichelt <v.reichelt@netcologne.de> > > * parser.c (cp_parser_decl_specifier_seq): Add fix-it hints for > friend outside class and obsolete auto as storage-class-specifier. > > Index: gcc/cp/parser.c > =================================================================== > --- gcc/cp/parser.c 2017-04-28 > +++ gcc/cp/parser.c 2017-04-28 > @@ -13213,7 +13213,9 @@ > case RID_FRIEND: > if (!at_class_scope_p ()) > { > - error_at (token->location, "%<friend%> used outside of class"); > + gcc_rich_location richloc (token->location); > + richloc.add_fixit_remove (); > + error_at_rich_loc (&richloc, "%<friend%> used outside of class"); > cp_lexer_purge_token (parser->lexer); > } > else > @@ -13277,8 +13279,11 @@ > > /* Complain about `auto' as a storage specifier, if > we're complaining about C++0x compatibility. */ > - warning_at (token->location, OPT_Wc__11_compat, "%<auto%>" > - " changes meaning in C++11; please remove it"); > + gcc_rich_location richloc (token->location); > + richloc.add_fixit_remove (); > + warning_at_rich_loc (&richloc, OPT_Wc__11_compat, > + "%<auto%> changes meaning in C++11; " > + "please remove it"); What caught my eye here is the "please remove it" part :) Maybe it's me but it seems a little too forceful for a warning (despite the please). I would find a more conventional phrasing like "suggest to remove it" to be more suitable. That said, I wonder if removing the auto is actually the best suggestion. Wouldn't it more in line with where C++ is headed to suggest to remove the type and keep the auto? Martin
On 2 May, Martin Sebor wrote: > On 04/29/2017 04:23 PM, Volker Reichelt wrote: >> Hi, >> >> the following patch adds fix-it hints to the C++ parser for two wrongly >> used keywords detected in cp_parser_decl_specifier_seq. >> >> Bootstrapped and regtested on x86_64-pc-linux-gnu. >> >> OK for trunk? >> >> Regards, >> Volker >> >> >> 2017-04-29 Volker Reichelt <v.reichelt@netcologne.de> >> >> * parser.c (cp_parser_decl_specifier_seq): Add fix-it hints for >> friend outside class and obsolete auto as storage-class-specifier. >> >> Index: gcc/cp/parser.c >> =================================================================== >> --- gcc/cp/parser.c 2017-04-28 >> +++ gcc/cp/parser.c 2017-04-28 >> @@ -13213,7 +13213,9 @@ >> case RID_FRIEND: >> if (!at_class_scope_p ()) >> { >> - error_at (token->location, "%<friend%> used outside of class"); >> + gcc_rich_location richloc (token->location); >> + richloc.add_fixit_remove (); >> + error_at_rich_loc (&richloc, "%<friend%> used outside of class"); >> cp_lexer_purge_token (parser->lexer); >> } >> else >> @@ -13277,8 +13279,11 @@ >> >> /* Complain about `auto' as a storage specifier, if >> we're complaining about C++0x compatibility. */ >> - warning_at (token->location, OPT_Wc__11_compat, "%<auto%>" >> - " changes meaning in C++11; please remove it"); >> + gcc_rich_location richloc (token->location); >> + richloc.add_fixit_remove (); >> + warning_at_rich_loc (&richloc, OPT_Wc__11_compat, >> + "%<auto%> changes meaning in C++11; " >> + "please remove it"); > > What caught my eye here is the "please remove it" part :) Maybe > it's me but it seems a little too forceful for a warning (despite > the please). I would find a more conventional phrasing like > "suggest to remove it" to be more suitable. > > That said, I wonder if removing the auto is actually the best > suggestion. Wouldn't it more in line with where C++ is headed > to suggest to remove the type and keep the auto? > > Martin I think you missed that we are in C++98 mode here. Dropping the 'auto' is the only viable choice to stay in C++98 mode and gain C++11 compatibility. With that in mind, I don't think that the wording "please remove it" is bad. The user is in C++98 mode and wants to know about C++11 compatibility (as OPT_Wc__11_compat is selected). So the compiler gives her/him clear advice what to do. For me "suggest to remove it" sounde too weak (more like "if you run into problems in C++11, you could try to drop the 'auto' here"). Other solutions like "'auto' needs to be removed here to gain C++11 compatibility" sound a bit clumsy to me. Regards, Volker
Index: gcc/cp/parser.c =================================================================== --- gcc/cp/parser.c 2017-04-28 +++ gcc/cp/parser.c 2017-04-28 @@ -13213,7 +13213,9 @@ case RID_FRIEND: if (!at_class_scope_p ()) { - error_at (token->location, "%<friend%> used outside of class"); + gcc_rich_location richloc (token->location); + richloc.add_fixit_remove (); + error_at_rich_loc (&richloc, "%<friend%> used outside of class"); cp_lexer_purge_token (parser->lexer); } else @@ -13277,8 +13279,11 @@ /* Complain about `auto' as a storage specifier, if we're complaining about C++0x compatibility. */ - warning_at (token->location, OPT_Wc__11_compat, "%<auto%>" - " changes meaning in C++11; please remove it"); + gcc_rich_location richloc (token->location); + richloc.add_fixit_remove (); + warning_at_rich_loc (&richloc, OPT_Wc__11_compat, + "%<auto%> changes meaning in C++11; " + "please remove it"); /* Set the storage class anyway. */ cp_parser_set_storage_class (parser, decl_specs, RID_AUTO, =================================================================== 2017-04-29 Volker Reichelt <v.reichelt@netcologne.de> * g++.dg/diagnostic/friend1.C: New test. * g++.dg/cpp0x/auto1.C: Add check for fix-it hint. Index: gcc/testsuite/g++.dg/diagnostic/friend1.C =================================================================== --- gcc/testsuite/g++.dg/diagnostic/friend1.C 2017-04-29 +++ gcc/testsuite/g++.dg/diagnostic/friend1.C 2017-04-29 @@ -0,0 +1,8 @@ +// { dg-options "-fdiagnostics-show-caret" } + +friend void foo(); /* { dg-error "used outside of class" } + { dg-begin-multiline-output "" } + friend void foo(); + ^~~~~~ + ------ + { dg-end-multiline-output "" } */ Index: gcc/testsuite/g++.dg/cpp0x/auto1.C =================================================================== --- gcc/testsuite/g++.dg/cpp0x/auto1.C (revision 247406) +++ gcc/testsuite/g++.dg/cpp0x/auto1.C (working copy) @@ -1,9 +1,14 @@ // { dg-do compile { target c++11 } } -// { dg-options "-std=c++98 -Wc++11-compat" } +// { dg-options "-std=c++98 -Wc++11-compat -fdiagnostics-show-caret" } // Test warning for use of auto in C++98 mode with C++11 // compatibility warnings void f() { - auto int x = 5; // { dg-warning "changes meaning" } + auto int x = 5; /* { dg-warning "changes meaning" } + { dg-begin-multiline-output "" } + auto int x = 5; + ^~~~ + ---- + { dg-end-multiline-output "" } */ }