[PATCH] c++ parser: fix-it hints for wrong usage of 'friend' and 'auto'

[PATCH] c++ parser: fix-it hints for wrong usage of 'friend' and 'auto'

[Volker Reichelt]

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");
 
               /* 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 "" } */
 }
===================================================================

Re: [PATCH] c++ parser: fix-it hints for wrong usage of 'friend' and 'auto'

[Martin Sebor]

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

Re: [PATCH] c++ parser: fix-it hints for wrong usage of 'friend' and 'auto'

[Volker Reichelt]

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