Re: [PATCH] c++: Improve diagnostics about conflicting specifiers

[Jason Merrill <jason@redhat.com>]

On 9/19/22 03:24, Jakub Jelinek wrote:
> Hi!
> 
> On Sat, Sep 17, 2022 at 01:23:59AM +0200, Jason Merrill wrote:
>> I wonder why we don't give an error when setting the
>> conflicting_specifiers_p flag in cp_parser_set_storage_class?  We should be
>> able to give a better diagnostic at that point.
> 
> I didn't have time to update the whole patch last night, but this part
> seems to be independent and I've managed to test it.
> 
> The diagnostics then looks like:
> a.C:1:9: error: ‘static’ specifier conflicts with ‘typedef’
>      1 | typedef static int a;
>        | ~~~~~~~ ^~~~~~
> a.C:2:8: error: ‘typedef’ specifier conflicts with ‘static’
>      2 | static typedef int b;
>        | ~~~~~~ ^~~~~~~
> a.C:3:8: error: duplicate ‘static’ specifier
>      3 | static static int c;
>        | ~~~~~~ ^~~~~~
> a.C:4:8: error: ‘extern’ specifier conflicts with ‘static’
>      4 | static extern int d;
>        | ~~~~~~ ^~~~~~
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

> 2022-09-19  Jakub Jelinek  <jakub@redhat.com>
> 
> gcc/cp/
> 	* parser.cc (cp_parser_lambda_declarator_opt): Don't diagnose
> 	conflicting specifiers here.
> 	(cp_storage_class_name): New variable.
> 	(cp_parser_decl_specifier_seq): When setting conflicting_specifiers_p
> 	for the first time, diagnose which exact specifiers conflict.
> 	(cp_parser_set_storage_class): Likewise.  Move storage_class
> 	computation earlier.
> 	* decl.cc (grokdeclarator): Don't diagnose conflicting specifiers
> 	here, just return error_mark_node.
> gcc/testsuite/
> 	* g++.dg/diagnostic/conflicting-specifiers-1.C: Adjust expected
> 	diagnostics.
> 	* g++.dg/parse/typedef8.C: Likewise.
> 	* g++.dg/parse/crash39.C: Likewise.
> 	* g++.dg/other/mult-stor1.C: Likewise.
> 	* g++.dg/cpp2a/constinit3.C: Likewise.
> 
> --- gcc/cp/parser.cc.jj	2022-09-16 22:34:28.427708581 +0200
> +++ gcc/cp/parser.cc	2022-09-19 09:18:49.561145347 +0200
> @@ -11718,9 +11718,6 @@ cp_parser_lambda_declarator_opt (cp_pars
>       {
>         LAMBDA_EXPR_MUTABLE_P (lambda_expr) = 1;
>         quals = TYPE_UNQUALIFIED;
> -      if (lambda_specs.conflicting_specifiers_p)
> -	error_at (lambda_specs.locations[ds_storage_class],
> -		  "duplicate %<mutable%>");
>       }
>   
>     tx_qual = cp_parser_tx_qualifier_opt (parser);
> @@ -15720,6 +15717,13 @@ cp_parser_decomposition_declaration (cp_
>     return decl;
>   }
>   
> +/* Names of storage classes.  */
> +
> +static const char *const
> +cp_storage_class_name[] = {
> +  "", "auto", "register", "static", "extern", "mutable"
> +};
> +
>   /* Parse a decl-specifier-seq.
>   
>      decl-specifier-seq:
> @@ -15941,8 +15945,18 @@ cp_parser_decl_specifier_seq (cp_parser*
>   	     may as well commit at this point.  */
>   	  cp_parser_commit_to_tentative_parse (parser);
>   
> -          if (decl_specs->storage_class != sc_none)
> -            decl_specs->conflicting_specifiers_p = true;
> +	  if (decl_specs->storage_class != sc_none)
> +	    {
> +	      if (decl_specs->conflicting_specifiers_p)
> +		break;
> +	      gcc_rich_location richloc (token->location);
> +	      location_t oloc = decl_specs->locations[ds_storage_class];
> +	      richloc.add_location_if_nearby (oloc);
> +	      error_at (&richloc,
> +			"%<typedef%> specifier conflicts with %qs",
> +			cp_storage_class_name[decl_specs->storage_class]);
> +	      decl_specs->conflicting_specifiers_p = true;
> +	    }
>   	  break;
>   
>   	  /* storage-class-specifier:
> @@ -32826,26 +32840,6 @@ cp_parser_set_storage_class (cp_parser *
>   {
>     cp_storage_class storage_class;
>   
> -  if (parser->in_unbraced_linkage_specification_p)
> -    {
> -      error_at (token->location, "invalid use of %qD in linkage specification",
> -		ridpointers[keyword]);
> -      return;
> -    }
> -  else if (decl_specs->storage_class != sc_none)
> -    {
> -      decl_specs->conflicting_specifiers_p = true;
> -      return;
> -    }
> -
> -  if ((keyword == RID_EXTERN || keyword == RID_STATIC)
> -      && decl_spec_seq_has_spec_p (decl_specs, ds_thread)
> -      && decl_specs->gnu_thread_keyword_p)
> -    {
> -      pedwarn (decl_specs->locations[ds_thread], 0,
> -		"%<__thread%> before %qD", ridpointers[keyword]);
> -    }
> -
>     switch (keyword)
>       {
>       case RID_AUTO:
> @@ -32866,6 +32860,38 @@ cp_parser_set_storage_class (cp_parser *
>       default:
>         gcc_unreachable ();
>       }
> +
> +  if (parser->in_unbraced_linkage_specification_p)
> +    {
> +      error_at (token->location, "invalid use of %qD in linkage specification",
> +		ridpointers[keyword]);
> +      return;
> +    }
> +  else if (decl_specs->storage_class != sc_none)
> +    {
> +      if (decl_specs->conflicting_specifiers_p)
> +	return;
> +      gcc_rich_location richloc (token->location);
> +      richloc.add_location_if_nearby (decl_specs->locations[ds_storage_class]);
> +      if (decl_specs->storage_class == storage_class)
> +	error_at (&richloc, "duplicate %qD specifier", ridpointers[keyword]);
> +      else
> +	error_at (&richloc,
> +		  "%qD specifier conflicts with %qs",
> +		  ridpointers[keyword],
> +		  cp_storage_class_name[decl_specs->storage_class]);
> +      decl_specs->conflicting_specifiers_p = true;
> +      return;
> +    }
> +
> +  if ((keyword == RID_EXTERN || keyword == RID_STATIC)
> +      && decl_spec_seq_has_spec_p (decl_specs, ds_thread)
> +      && decl_specs->gnu_thread_keyword_p)
> +    {
> +      pedwarn (decl_specs->locations[ds_thread], 0,
> +		"%<__thread%> before %qD", ridpointers[keyword]);
> +    }
> +
>     decl_specs->storage_class = storage_class;
>     set_and_check_decl_spec_loc (decl_specs, ds_storage_class, token);
>   
> @@ -32873,8 +32899,16 @@ cp_parser_set_storage_class (cp_parser *
>        specifier. If there is a typedef specifier present then set
>        conflicting_specifiers_p which will trigger an error later
>        on in grokdeclarator. */
> -  if (decl_spec_seq_has_spec_p (decl_specs, ds_typedef))
> -    decl_specs->conflicting_specifiers_p = true;
> +  if (decl_spec_seq_has_spec_p (decl_specs, ds_typedef)
> +      && !decl_specs->conflicting_specifiers_p)
> +    {
> +      gcc_rich_location richloc (token->location);
> +      richloc.add_location_if_nearby (decl_specs->locations[ds_typedef]);
> +      error_at (&richloc,
> +		"%qD specifier conflicts with %<typedef%>",
> +		ridpointers[keyword]);
> +      decl_specs->conflicting_specifiers_p = true;
> +    }
>   }
>   
>   /* Update the DECL_SPECS to reflect the TYPE_SPEC.  If TYPE_DEFINITION_P
> --- gcc/cp/decl.cc.jj	2022-09-16 22:34:28.420708676 +0200
> +++ gcc/cp/decl.cc	2022-09-18 20:15:13.357162931 +0200
> @@ -12089,12 +12089,7 @@ grokdeclarator (const cp_declarator *dec
>       }
>   
>     if (declspecs->conflicting_specifiers_p)
> -    {
> -      error_at (min_location (declspecs->locations[ds_typedef],
> -			      declspecs->locations[ds_storage_class]),
> -		"conflicting specifiers in declaration of %qs", name);
> -      return error_mark_node;
> -    }
> +    return error_mark_node;
>   
>     /* Extract the basic type from the decl-specifier-seq.  */
>     type = declspecs->type;
> --- gcc/testsuite/g++.dg/diagnostic/conflicting-specifiers-1.C.jj	2020-01-14 20:02:46.815609354 +0100
> +++ gcc/testsuite/g++.dg/diagnostic/conflicting-specifiers-1.C	2022-09-18 20:55:53.928964842 +0200
> @@ -1 +1 @@
> -static typedef int i __attribute__((unused));  // { dg-error "1:conflicting specifiers" }
> +static typedef int i __attribute__((unused));  // { dg-error "1:'typedef' specifier conflicts with 'static'" }
> --- gcc/testsuite/g++.dg/parse/typedef8.C.jj	2020-01-14 20:02:46.921607767 +0100
> +++ gcc/testsuite/g++.dg/parse/typedef8.C	2022-09-18 20:55:23.829375095 +0200
> @@ -1,11 +1,11 @@
>   //PR c++ 29024
>   
> -typedef static int a;   // { dg-error "conflicting" }
> -typedef register int b; // { dg-error "conflicting" }
> -typedef extern int c;   // { dg-error "conflicting" }
> -static typedef int a;   // { dg-error "conflicting" }
> +typedef static int a;   // { dg-error "'static' specifier conflicts with 'typedef'" }
> +typedef register int b; // { dg-error "'register' specifier conflicts with 'typedef'" }
> +typedef extern int c;   // { dg-error "'extern' specifier conflicts with 'typedef'" }
> +static typedef int a;   // { dg-error "'typedef' specifier conflicts with 'static'" }
>   
>   void foo()
>   {
> -  typedef auto int bar; // { dg-error "conflicting|two or more data types" }
> +  typedef auto int bar; // { dg-error "'auto' specifier conflicts with 'typedef'|two or more data types" }
>   }
> --- gcc/testsuite/g++.dg/parse/crash39.C.jj	2020-01-14 20:02:46.911607917 +0100
> +++ gcc/testsuite/g++.dg/parse/crash39.C	2022-09-18 20:56:32.004445908 +0200
> @@ -1,3 +1,3 @@
>   // PR c++/31747
>   
> -static extern int i; // { dg-error "conflicting specifiers" }
> +static extern int i; // { dg-error "'extern' specifier conflicts with 'static'" }
> --- gcc/testsuite/g++.dg/other/mult-stor1.C.jj	2020-01-14 20:02:46.902608051 +0100
> +++ gcc/testsuite/g++.dg/other/mult-stor1.C	2022-09-18 20:57:43.635469614 +0200
> @@ -4,5 +4,5 @@
>   
>   struct A
>   {
> -  extern static int i;  // { dg-error "conflicting specifiers" }
> +  extern static int i;  // { dg-error "'static' specifier conflicts with 'extern'" }
>   };
> --- gcc/testsuite/g++.dg/cpp2a/constinit3.C.jj	2020-05-13 21:38:28.356420335 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/constinit3.C	2022-09-18 20:57:12.424894986 +0200
> @@ -5,7 +5,7 @@ constinit constinit int v1; // { dg-erro
>   constexpr constinit int v2 = 1; // { dg-error "can use at most one of the .constinit. and .constexpr. specifiers" }
>   constinit constexpr int v3 = 1; // { dg-error "an use at most one of the .constinit. and .constexpr. specifiers" }
>   
> -extern static constinit int v4; // { dg-error "conflicting specifiers" }
> +extern static constinit int v4; // { dg-error "'static' specifier conflicts with 'extern'" }
>   extern thread_local constinit int v5;
>   extern constinit int v6;
>   
> 
> 	Jakub
>