[C++] fix some wrong decl slocs

This patch fixes several cases of incorrect location used for declarations,
because the g++ parser makes a heavy use of 'input_location'.

The approach taken here is to try to be as less disruptive/intrusive as
possible, so we first set declarator->id_loc in various make_*_declarator
functions, and then in cp_parser_init_declarator, we use this setting
in favor of input_location. We need to check this because in case of a
redeclaration (call to merge_decls), we do not want to modify the location.

This patch was written in the context of a larger project where I'm modifying
gcc to generate xref info from the gcc tree, and for the below source, the
decl locations where incorrect (past the variable name):

char * d [10];
char e [15][10];
int (*f)();

I've transformed the above into a g++ test case where we force a compiler
error on a redefinition of d. Prior to this patch, the second error message was:

redef2.C:3:13: error: 'd' has a previous declaration as 'char* d [10]'

and with this patch we now get:

redef2.C:3:8: error: 'd' has a previous declaration as 'char* d [10]'

Tested on x86_64-pc-linux-gnu, OK for trunk?

gcc/cp/

2010-08-30  Arnaud Charlet  <charlet@adacore.com>

	* parser.c (make_pointer_declarator, make_reference_declarator,
	make_call_declarator, make_array_declarator): Set declarator->id_loc.
	(cp_parser_init_declarator): Adjust location of decl if appropriate.

testsuite/
2010-08-30  Arnaud Charlet  <charlet@adacore.com>

	* g++.dg/parse/redef2.C: New.

ping :-)

http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02089.html

On Mon, Aug 30, 2010 at 03:11:50PM +0200, Arnaud Charlet wrote:
> This patch fixes several cases of incorrect location used for
> declarations,
> because the g++ parser makes a heavy use of 'input_location'.
> 
> The approach taken here is to try to be as less disruptive/intrusive as
> possible, so we first set declarator->id_loc in various
> make_*_declarator
> functions, and then in cp_parser_init_declarator, we use this setting
> in favor of input_location. We need to check this because in case of a
> redeclaration (call to merge_decls), we do not want to modify the location.
> 
> This patch was written in the context of a larger project where I'm modifying
> gcc to generate xref info from the gcc tree, and for the below source, the
> decl locations where incorrect (past the variable name):
> 
> char * d [10];
> char e [15][10];
> int (*f)();
> 
> I've transformed the above into a g++ test case where we force a compiler
> error on a redefinition of d. Prior to this patch, the second error message
> was:
> 
> redef2.C:3:13: error: 'd' has a previous declaration as 'char* d [10]'
> 
> and with this patch we now get:
> 
> redef2.C:3:8: error: 'd' has a previous declaration as 'char* d [10]'
> 
> Tested on x86_64-pc-linux-gnu, OK for trunk?
> 
> gcc/cp/
> 
> 2010-08-30  Arnaud Charlet  <charlet@adacore.com>
> 
> 	* parser.c (make_pointer_declarator, make_reference_declarator,
> 	make_call_declarator, make_array_declarator): Set
> 	declarator->id_loc.
> 	(cp_parser_init_declarator): Adjust location of decl if appropriate.
> 
> testsuite/
> 2010-08-30  Arnaud Charlet  <charlet@adacore.com>
> 
> 	* g++.dg/parse/redef2.C: New.
> 
> --- gcc/cp/parser.c	(revision 163636)
> +++ gcc/cp/parser.c	(working copy)
> @@ -1068,6 +1068,7 @@ make_pointer_declarator (cp_cv_quals cv_
>    declarator->u.pointer.class_type = NULL_TREE;
>    if (target)
>      {
> +      declarator->id_loc = target->id_loc;
>        declarator->parameter_pack_p = target->parameter_pack_p;
>        target->parameter_pack_p = false;
>      }
> @@ -1091,6 +1092,7 @@ make_reference_declarator (cp_cv_quals c
>    declarator->u.reference.rvalue_ref = rvalue_ref;
>    if (target)
>      {
> +      declarator->id_loc = target->id_loc;
>        declarator->parameter_pack_p = target->parameter_pack_p;
>        target->parameter_pack_p = false;
>      }
> @@ -1147,6 +1149,7 @@ make_call_declarator (cp_declarator *tar
>    declarator->u.function.late_return_type = late_return_type;
>    if (target)
>      {
> +      declarator->id_loc = target->id_loc;
>        declarator->parameter_pack_p = target->parameter_pack_p;
>        target->parameter_pack_p = false;
>      }
> @@ -1169,6 +1172,7 @@ make_array_declarator (cp_declarator *el
>    declarator->u.array.bounds = bounds;
>    if (element)
>      {
> +      declarator->id_loc = element->id_loc;
>        declarator->parameter_pack_p = element->parameter_pack_p;
>        element->parameter_pack_p = false;
>      }
> @@ -14010,6 +14014,13 @@ cp_parser_init_declarator (cp_parser* pa
>        decl = start_decl (declarator, decl_specifiers,
>  			 is_initialized, attributes, prefix_attributes,
>  			 &pushed_scope);
> +      /* Adjust location of decl if declarator->id_loc is more appropriate:
> +	 set, and decl wasn't merged with another decl, in which case its
> +	 location would be different from input_location, and more accurate.  */
> +      if (DECL_P (decl)
> +	  && declarator->id_loc != UNKNOWN_LOCATION
> +	  && DECL_SOURCE_LOCATION (decl) == input_location)
> +	DECL_SOURCE_LOCATION (decl) = declarator->id_loc;
>      }
>    else if (scope)
>      /* Enter the SCOPE.  That way unqualified names appearing in the
> 
> --- testsuite/g++.dg/parse/redef2.C	(revision 0)
> +++ testsuite/g++.dg/parse/redef2.C	(revision 0)
> @@ -0,0 +1,7 @@
> +// { dg-do assemble }
> +
> +char * d [10];  // { dg-error "8: 'd' has a previous declaration as" }
> +char e [15][10];
> +int (*f)();
> +
> +int d;  // { dg-error "" }
>

OK, thanks.

Jason

On 9/6/2010 2:24 AM, Arnaud Charlet wrote:

>> I've transformed the above into a g++ test case where we force a compiler
>> error on a redefinition of d. Prior to this patch, the second error message
>> was:
>>
>> redef2.C:3:13: error: 'd' has a previous declaration as 'char* d [10]'
>>
>> and with this patch we now get:
>>
>> redef2.C:3:8: error: 'd' has a previous declaration as 'char* d [10]'

The idea of this patch is very good.  Would you please post the complete
error output for the test case?  I want to see that there is still a
reference to line three; there really should be references to both lines
so that the user knows both where the redeclaration occurred and where
the original declaration occurred.

Also, the test case doesn't need to be:

>> +// { dg-do assemble }

Just "// { dg-do compile }".  (It's a minor point, but no reason to make
the testsuite any more expensive than it needs to be.)

Thanks,

> The idea of this patch is very good.  Would you please post the complete
> error output for the test case?  I want to see that there is still a
> reference to line three; there really should be references to both lines
> so that the user knows both where the redeclaration occurred and where
> the original declaration occurred.

The complete output is:

$ g++ -c redef2.C
redef2.C:7:5: error: conflicting declaration 'int d'
redef2.C:3:8: error: 'd' has a previous declaration as 'char* d [10]'

Only the column number has changed, nothing else.

> Also, the test case doesn't need to be:
> 
> >> +// { dg-do assemble }
> 
> Just "// { dg-do compile }".  (It's a minor point, but no reason to make
> the testsuite any more expensive than it needs to be.)

OK, I've fixed that. Thanks for the review.

Arno

On 9/6/2010 8:57 AM, Arnaud Charlet wrote:

> $ g++ -c redef2.C
> redef2.C:7:5: error: conflicting declaration 'int d'
> redef2.C:3:8: error: 'd' has a previous declaration as 'char* d [10]'
> 
> Only the column number has changed, nothing else.

Oh, I see!

Patch is OK with dg-compile change.

Thanks,