[C++] Further grokdeclarator locations work

Hi,

only minor functional changes here - more precise locations for two 
error messages  - but I think it's a step in the right direction: it 
moves the declaration of id_loc way up, near typespec_loc and as such 
id_loc is immediately used. Then its value is updated upon the loop over 
declarator in the middle of the function and used again in the final 
part of the function. That also "frees" the simple name loc for other 
local uses, allows to simplify those checks changed rather recently over 
(name && declarator). and (unqualified_id && declarator). Tested 
x86_64-linux.

Thanks, Paolo.

//////////////////////
/cp
2019-06-06  Paolo Carlini  <paolo.carlini@oracle.com>

	* decl.c (grokdeclarator): Move further up the declaration of
	id_loc, use it immediately, update its value after the loop
	over declarator, use it again in the final part of function;
	use id_loc in error messages about multiple data types and
	conflicting specifiers.

/testsuite
2019-06-06  Paolo Carlini  <paolo.carlini@oracle.com>

	* g++.dg/diagnostic/conflicting-specifiers-1.C: New.
	* g++.dg/diagnostic/two-or-more-data-types-1.C: Likewise.

On 6/6/19 9:00 AM, Paolo Carlini wrote:
> Hi,
> 
> only minor functional changes here - more precise locations for two 
> error messages  - but I think it's a step in the right direction: it 
> moves the declaration of id_loc way up, near typespec_loc and as such 
> id_loc is immediately used. Then its value is updated upon the loop over 
> declarator in the middle of the function and used again in the final 
> part of the function. That also "frees" the simple name loc for other 
> local uses, allows to simplify those checks changed rather recently over 
> (name && declarator). and (unqualified_id && declarator). Tested 
> x86_64-linux.

Mostly OK, except:

>    if (declspecs->multiple_types_p)
>      {
> -      error ("two or more data types in declaration of %qs", name);
> +      error_at (id_loc, "two or more data types in declaration of %qs", name);
>        return error_mark_node;
>      }

declspecs->locations[ds_type_spec]?

>    if (declspecs->conflicting_specifiers_p)
>      {
> -      error ("conflicting specifiers in declaration of %qs", name);
> +      error_at (id_loc, "conflicting specifiers in declaration of %qs", name);
>        return error_mark_node;
>      }

ds_storage_class/ds_typedef?

Jason

Hi,

On 12/06/19 22:24, Jason Merrill wrote:
> On 6/6/19 9:00 AM, Paolo Carlini wrote:
>> Hi,
>>
>> only minor functional changes here - more precise locations for two 
>> error messages  - but I think it's a step in the right direction: it 
>> moves the declaration of id_loc way up, near typespec_loc and as such 
>> id_loc is immediately used. Then its value is updated upon the loop 
>> over declarator in the middle of the function and used again in the 
>> final part of the function. That also "frees" the simple name loc for 
>> other local uses, allows to simplify those checks changed rather 
>> recently over (name && declarator). and (unqualified_id && 
>> declarator). Tested x86_64-linux.
>
> Mostly OK, except:
>
>>    if (declspecs->multiple_types_p)
>>      {
>> -      error ("two or more data types in declaration of %qs", name);
>> +      error_at (id_loc, "two or more data types in declaration of 
>> %qs", name);
>>        return error_mark_node;
>>      }
>
> declspecs->locations[ds_type_spec]?
>
>>    if (declspecs->conflicting_specifiers_p)
>>      {
>> -      error ("conflicting specifiers in declaration of %qs", name);
>> +      error_at (id_loc, "conflicting specifiers in declaration of 
>> %qs", name);
>>        return error_mark_node;
>>      }
>
> ds_storage_class/ds_typedef?

Ok.

Ideally, it would be nice to have available locations for the two 
leftmost data types and, for the second error, the locations of 
conflicting storage classes (eg, static and register). Other compilers 
then point to the second one, where the issue shows up. I think we 
already briefly discussed that kind of logic in the past, but in those 
cases we had available all the locations we needed and we ended up using 
gcc_rich_location and add_range, etc..

Anyway, we can certainly use typespec_loc and the minimum of 
ds_storage_class and ds_typedef, respectively, like in the below, which 
passes testing.

Thanks! Paolo.

///////////////////
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 272213)
+++ cp/decl.c	(working copy)
@@ -10456,6 +10456,8 @@ grokdeclarator (const cp_declarator *declarator,
   if (typespec_loc == UNKNOWN_LOCATION)
     typespec_loc = input_location;
 
+  location_t id_loc = declarator ? declarator->id_loc : input_location;
+
   /* Look inside a declarator for the name being declared
      and get it as a string, for an error message.  */
   for (id_declarator = declarator;
@@ -10620,8 +10622,7 @@ grokdeclarator (const cp_declarator *declarator,
      D1 ( parameter-declaration-clause) ...  */
   if (funcdef_flag && innermost_code != cdk_function)
     {
-      error_at (declarator->id_loc,
-		"function definition does not declare parameters");
+      error_at (id_loc, "function definition does not declare parameters");
       return error_mark_node;
     }
 
@@ -10629,8 +10630,7 @@ grokdeclarator (const cp_declarator *declarator,
       && innermost_code != cdk_function
       && ! (ctype && !declspecs->any_specifiers_p))
     {
-      error_at (declarator->id_loc,
-		"declaration of %qD as non-function", dname);
+      error_at (id_loc, "declaration of %qD as non-function", dname);
       return error_mark_node;
     }
 
@@ -10639,8 +10639,7 @@ grokdeclarator (const cp_declarator *declarator,
       if (UDLIT_OPER_P (dname)
 	  && innermost_code != cdk_function)
 	{
-	  error_at (declarator->id_loc,
-		    "declaration of %qD as non-function", dname);
+	  error_at (id_loc, "declaration of %qD as non-function", dname);
 	  return error_mark_node;
 	}
 
@@ -10648,14 +10647,12 @@ grokdeclarator (const cp_declarator *declarator,
 	{
 	  if (typedef_p)
 	    {
-	      error_at (declarator->id_loc,
-			"declaration of %qD as %<typedef%>", dname);
+	      error_at (id_loc, "declaration of %qD as %<typedef%>", dname);
 	      return error_mark_node;
 	    }
 	  else if (decl_context == PARM || decl_context == CATCHPARM)
 	    {
-	      error_at (declarator->id_loc,
-			"declaration of %qD as parameter", dname);
+	      error_at (id_loc, "declaration of %qD as parameter", dname);
 	      return error_mark_node;
 	    }
 	}
@@ -10705,13 +10702,16 @@ grokdeclarator (const cp_declarator *declarator,
      issue an error message.  */
   if (declspecs->multiple_types_p)
     {
-      error ("two or more data types in declaration of %qs", name);
+      error_at (typespec_loc,
+		"two or more data types in declaration of %qs", name);
       return error_mark_node;
     }
 
   if (declspecs->conflicting_specifiers_p)
     {
-      error ("conflicting specifiers in declaration of %qs", name);
+      error_at (min_location (declspecs->locations[ds_typedef],
+			      declspecs->locations[ds_storage_class]),
+		"conflicting specifiers in declaration of %qs", name);
       return error_mark_node;
     }
 
@@ -11861,6 +11861,8 @@ grokdeclarator (const cp_declarator *declarator,
 	}
     }
 
+  id_loc = declarator ? declarator->id_loc : input_location;
+
   /* A `constexpr' specifier used in an object declaration declares
      the object as `const'.  */
   if (constexpr_p && innermost_code != cdk_function)
@@ -11884,8 +11886,6 @@ grokdeclarator (const cp_declarator *declarator,
       unqualified_id = dname;
     }
 
-  location_t loc = declarator ? declarator->id_loc : input_location;
-
   /* If TYPE is a FUNCTION_TYPE, but the function name was explicitly
      qualified with a class-name, turn it into a METHOD_TYPE, unless
      we know that the function is static.  We take advantage of this
@@ -11912,7 +11912,7 @@ grokdeclarator (const cp_declarator *declarator,
 	      friendp = 0;
 	    }
 	  else
-	    permerror (loc, "extra qualification %<%T::%> on member %qs",
+	    permerror (id_loc, "extra qualification %<%T::%> on member %qs",
 		       ctype, name);
 	}
       else if (/* If the qualifying type is already complete, then we
@@ -11941,7 +11941,7 @@ grokdeclarator (const cp_declarator *declarator,
 	  if (current_class_type
 	      && (!friendp || funcdef_flag || initialized))
 	    {
-	      error_at (loc, funcdef_flag || initialized
+	      error_at (id_loc, funcdef_flag || initialized
 			? G_("cannot define member function %<%T::%s%> "
 			     "within %qT")
 			: G_("cannot declare member function %<%T::%s%> "
@@ -11952,7 +11952,7 @@ grokdeclarator (const cp_declarator *declarator,
 	}
       else if (typedef_p && current_class_type)
 	{
-	  error_at (loc, "cannot declare member %<%T::%s%> within %qT",
+	  error_at (id_loc, "cannot declare member %<%T::%s%> within %qT",
 		    ctype, name, current_class_type);
 	  return error_mark_node;
 	}
@@ -12111,9 +12111,9 @@ grokdeclarator (const cp_declarator *declarator,
 	}
 
       if (decl_context == FIELD)
-	decl = build_lang_decl_loc (loc, TYPE_DECL, unqualified_id, type);
+	decl = build_lang_decl_loc (id_loc, TYPE_DECL, unqualified_id, type);
       else
-	decl = build_decl (loc, TYPE_DECL, unqualified_id, type);
+	decl = build_decl (id_loc, TYPE_DECL, unqualified_id, type);
 
       if (decl_context != FIELD)
 	{
@@ -12398,8 +12398,8 @@ grokdeclarator (const cp_declarator *declarator,
 		if (in_system_header_at (input_location))
 		  /* Do not warn on flexible array members in system
 		     headers because glibc uses them.  */;
-		else if (name && declarator)
-		  pedwarn (declarator->id_loc, OPT_Wpedantic,
+		else if (name)
+		  pedwarn (id_loc, OPT_Wpedantic,
 			   "ISO C++ forbids flexible array member %qs", name);
 		else
 		  pedwarn (input_location, OPT_Wpedantic,
@@ -12551,7 +12551,7 @@ grokdeclarator (const cp_declarator *declarator,
 			       initialized == SD_DELETED, sfk,
 			       funcdef_flag, late_return_type_p,
 			       template_count, in_namespace,
-			       attrlist, declarator->id_loc);
+			       attrlist, id_loc);
             decl = set_virt_specifiers (decl, virt_specifiers);
 	    if (decl == NULL_TREE)
 	      return error_mark_node;
@@ -12584,8 +12584,7 @@ grokdeclarator (const cp_declarator *declarator,
 	      {
 		if (unqualified_id)
 		  {
-		    error_at (declarator->id_loc,
-			      "field %qD has incomplete type %qT",
+		    error_at (id_loc, "field %qD has incomplete type %qT",
 			      unqualified_id, type);
 		    cxx_incomplete_type_inform (strip_array_types (type));
 		  }
@@ -12600,8 +12599,8 @@ grokdeclarator (const cp_declarator *declarator,
 	  {
 	    if (friendp)
 	      {
-		if (unqualified_id && declarator)
-		  error_at (declarator->id_loc,
+		if (unqualified_id)
+		  error_at (id_loc,
 			    "%qE is neither function nor member function; "
 			    "cannot be declared friend", unqualified_id);
 		else
@@ -12645,7 +12644,7 @@ grokdeclarator (const cp_declarator *declarator,
 	      {
 		/* C++ allows static class members.  All other work
 		   for this is done by grokfield.  */
-		decl = build_lang_decl_loc (loc, VAR_DECL,
+		decl = build_lang_decl_loc (id_loc, VAR_DECL,
 					    unqualified_id, type);
 		set_linkage_for_static_data_member (decl);
 		if (concept_p)
@@ -12693,7 +12692,7 @@ grokdeclarator (const cp_declarator *declarator,
 			      unqualified_id);
 		    constexpr_p = false;
 		  }
-		decl = build_decl (loc, FIELD_DECL, unqualified_id, type);
+		decl = build_decl (id_loc, FIELD_DECL, unqualified_id, type);
 		DECL_NONADDRESSABLE_P (decl) = bitfield;
 		if (bitfield && !unqualified_id)
 		  {
@@ -12811,7 +12810,7 @@ grokdeclarator (const cp_declarator *declarator,
                            funcdef_flag,
 			   late_return_type_p,
 			   template_count, in_namespace, attrlist,
-			   declarator->id_loc);
+			   id_loc);
 	if (decl == NULL_TREE)
 	  return error_mark_node;
 
@@ -12858,7 +12857,7 @@ grokdeclarator (const cp_declarator *declarator,
 			    concept_p,
 			    template_count,
 			    ctype ? ctype : in_namespace,
-			    loc);
+			    id_loc);
 	if (decl == NULL_TREE)
 	  return error_mark_node;
 
@@ -12904,7 +12903,7 @@ grokdeclarator (const cp_declarator *declarator,
 	if (innermost_code == cdk_decomp)
 	  {
 	    gcc_assert (declarator && declarator->kind == cdk_decomp);
-	    DECL_SOURCE_LOCATION (decl) = declarator->id_loc;
+	    DECL_SOURCE_LOCATION (decl) = id_loc;
 	    DECL_ARTIFICIAL (decl) = 1;
 	    fit_decomposition_lang_decl (decl, NULL_TREE);
 	  }
Index: testsuite/g++.dg/diagnostic/conflicting-specifiers-1.C
===================================================================
--- testsuite/g++.dg/diagnostic/conflicting-specifiers-1.C	(nonexistent)
+++ testsuite/g++.dg/diagnostic/conflicting-specifiers-1.C	(working copy)
@@ -0,0 +1 @@
+static typedef int i __attribute__((unused));  // { dg-error "1:conflicting specifiers" }
Index: testsuite/g++.dg/diagnostic/two-or-more-data-types-1.C
===================================================================
--- testsuite/g++.dg/diagnostic/two-or-more-data-types-1.C	(nonexistent)
+++ testsuite/g++.dg/diagnostic/two-or-more-data-types-1.C	(working copy)
@@ -0,0 +1 @@
+char int i __attribute__((unused));  // { dg-error "1:two or more data types" }
Index: testsuite/g++.dg/diagnostic/variably-modified-type-1.C
===================================================================

On 6/12/19 5:36 PM, Paolo Carlini wrote:
> Hi,
> 
> On 12/06/19 22:24, Jason Merrill wrote:
>> On 6/6/19 9:00 AM, Paolo Carlini wrote:
>>> Hi,
>>>
>>> only minor functional changes here - more precise locations for two 
>>> error messages  - but I think it's a step in the right direction: it 
>>> moves the declaration of id_loc way up, near typespec_loc and as such 
>>> id_loc is immediately used. Then its value is updated upon the loop 
>>> over declarator in the middle of the function and used again in the 
>>> final part of the function. That also "frees" the simple name loc for 
>>> other local uses, allows to simplify those checks changed rather 
>>> recently over (name && declarator). and (unqualified_id && 
>>> declarator). Tested x86_64-linux.
>>
>> Mostly OK, except:
>>
>>>    if (declspecs->multiple_types_p)
>>>      {
>>> -      error ("two or more data types in declaration of %qs", name);
>>> +      error_at (id_loc, "two or more data types in declaration of 
>>> %qs", name);
>>>        return error_mark_node;
>>>      }
>>
>> declspecs->locations[ds_type_spec]?
>>
>>>    if (declspecs->conflicting_specifiers_p)
>>>      {
>>> -      error ("conflicting specifiers in declaration of %qs", name);
>>> +      error_at (id_loc, "conflicting specifiers in declaration of 
>>> %qs", name);
>>>        return error_mark_node;
>>>      }
>>
>> ds_storage_class/ds_typedef?
> 
> Ok.
> 
> Ideally, it would be nice to have available locations for the two 
> leftmost data types and, for the second error, the locations of 
> conflicting storage classes (eg, static and register). Other compilers 
> then point to the second one, where the issue shows up. I think we 
> already briefly discussed that kind of logic in the past, but in those 
> cases we had available all the locations we needed and we ended up using 
> gcc_rich_location and add_range, etc..
> 
> Anyway, we can certainly use typespec_loc and the minimum of 
> ds_storage_class and ds_typedef, respectively, like in the below, which 
> passes testing.
> 
> Thanks! Paolo.

OK.

Jason