[C PATCH] Fix up location used in get_parm_info diagnostics (PR c/68533)

[C PATCH] Fix up location used in get_parm_info diagnostics (PR c/68533)

[Jakub Jelinek]

Hi!

get_parm_info right now uses input_location as the diagnostics locus, but as
can be seen on the testcase, that is pretty random location at that point,
often the type of the last parameter.

This patch changes it to use the locus from the binding info.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2015-12-01  Jakub Jelinek  <jakub@redhat.com>

	PR c/68533
	* c-decl.c (get_parm_info): Use b->locus instead of input_location
	for diagnostics.

	* gcc.dg/pr68533.c: New test.

--- gcc/c/c-decl.c.jj	2015-11-30 13:40:35.000000000 +0100
+++ gcc/c/c-decl.c	2015-12-01 16:14:40.466462666 +0100
@@ -6913,11 +6913,11 @@ get_parm_info (bool ellipsis, tree expr)
     {
       if (TYPE_QUALS (TREE_TYPE (b->decl)) != TYPE_UNQUALIFIED
 	  || C_DECL_REGISTER (b->decl))
-	error ("%<void%> as only parameter may not be qualified");
+	error_at (b->locus, "%<void%> as only parameter may not be qualified");
 
       /* There cannot be an ellipsis.  */
       if (ellipsis)
-	error ("%<void%> must be the only parameter");
+	error_at (b->locus, "%<void%> must be the only parameter");
 
       arg_info->types = void_list_node;
       return arg_info;
@@ -6946,13 +6946,14 @@ get_parm_info (bool ellipsis, tree expr)
 
 	  /* Check for forward decls that never got their actual decl.  */
 	  if (TREE_ASM_WRITTEN (decl))
-	    error ("parameter %q+D has just a forward declaration", decl);
+	    error_at (b->locus,
+		      "parameter %q+D has just a forward declaration", decl);
 	  /* Check for (..., void, ...) and issue an error.  */
 	  else if (VOID_TYPE_P (type) && !DECL_NAME (decl))
 	    {
 	      if (!gave_void_only_once_err)
 		{
-		  error ("%<void%> must be the only parameter");
+		  error_at (b->locus, "%<void%> must be the only parameter");
 		  gave_void_only_once_err = true;
 		}
 	    }
@@ -6991,13 +6992,13 @@ get_parm_info (bool ellipsis, tree expr)
 	    {
 	      if (b->id)
 		/* The %s will be one of 'struct', 'union', or 'enum'.  */
-		warning_at (input_location, 0,
+		warning_at (b->locus, 0,
 			    "%<%s %E%> declared inside parameter list"
 			    " will not be visible outside of this definition or"
 			    " declaration", keyword, b->id);
 	      else
 		/* The %s will be one of 'struct', 'union', or 'enum'.  */
-		warning_at (input_location, 0,
+		warning_at (b->locus, 0,
 			    "anonymous %s declared inside parameter list"
 			    " will not be visible outside of this definition or"
 			    " declaration", keyword);
--- gcc/testsuite/gcc.dg/pr68533.c.jj	2015-12-01 18:47:55.864178841 +0100
+++ gcc/testsuite/gcc.dg/pr68533.c	2015-12-01 18:48:56.000000000 +0100
@@ -0,0 +1,68 @@
+/* PR c/68533 */
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+struct T { int t; };
+
+void
+f1 (
+  struct S *	/* { dg-warning "declared inside parameter list will not be visible outside of this definition or declaration" } */
+  x,
+  struct T *
+  y
+   )
+{
+  y->t = 4;
+}
+
+void
+f2 (
+  struct {int s;} * /* { dg-warning "anonymous struct declared inside parameter list will not be visible outside of this definition or declaration" } */
+  x,
+  struct T *
+  y
+   )
+{
+  y->t = 5;
+}
+
+void
+f3 (
+  const void
+   )		/* { dg-error "'void' as only parameter may not be qualified" } */
+{
+}
+
+void
+f4 (
+   void,	/* { dg-error "'void' must be the only parameter" } */
+   ...
+   )
+{
+}
+
+void
+f5 (
+   int
+   x;		/* { dg-error "parameter 'x' has just a forward declaration" } */
+   int y
+   )
+{
+}
+
+void
+f6 (
+   int
+   x,
+   void
+   )		/* { dg-error "'void' must be the only parameter" } */
+{
+}
+
+void
+f7 (
+   void,	/* { dg-error "'void' must be the only parameter" } */
+   int y
+   )
+{
+}

	Jakub

Re: [C PATCH] Fix up location used in get_parm_info diagnostics (PR c/68533)

[Jeff Law]

On 12/01/2015 01:34 PM, Jakub Jelinek wrote:
> Hi!
>
> get_parm_info right now uses input_location as the diagnostics locus, but as
> can be seen on the testcase, that is pretty random location at that point,
> often the type of the last parameter.
>
> This patch changes it to use the locus from the binding info.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2015-12-01  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR c/68533
> 	* c-decl.c (get_parm_info): Use b->locus instead of input_location
> 	for diagnostics.
>
> 	* gcc.dg/pr68533.c: New test.
I think the change itself is fine.  My question is whether or not the 
C++ front-end gets this right.  ISTM we ought to be running the test on 
both the C & C++ front-ends.  The C++ front-end may emit different 
messages, but we ought to be able to account for that and ensure that 
we're getting them on the right lines.

Jeff

Re: [C PATCH] Fix up location used in get_parm_info diagnostics (PR c/68533)

[Jakub Jelinek]

On Tue, Dec 01, 2015 at 08:57:39PM -0700, Jeff Law wrote:
> On 12/01/2015 01:34 PM, Jakub Jelinek wrote:
> >Hi!
> >
> >get_parm_info right now uses input_location as the diagnostics locus, but as
> >can be seen on the testcase, that is pretty random location at that point,
> >often the type of the last parameter.
> >
> >This patch changes it to use the locus from the binding info.
> >
> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >
> >2015-12-01  Jakub Jelinek  <jakub@redhat.com>
> >
> >	PR c/68533
> >	* c-decl.c (get_parm_info): Use b->locus instead of input_location
> >	for diagnostics.
> >
> >	* gcc.dg/pr68533.c: New test.
> I think the change itself is fine.  My question is whether or not the C++
> front-end gets this right.  ISTM we ought to be running the test on both the
> C & C++ front-ends.  The C++ front-end may emit different messages, but we
> ought to be able to account for that and ensure that we're getting them on
> the right lines.

The warning does not exist at all in the C++ FE, it has instead
just an error on declaring anonymous types (not others) among parameters.
C++ does not have forward parameter declarations.  And for the void
among arguments diagnostics it uses slightly different locations and
completely different wording.  So I'm afraid there is basically nothing in
common between C and C++ FEs in this area, so a shared testcase does not
make sense.

	Jakub

Re: [C PATCH] Fix up location used in get_parm_info diagnostics (PR c/68533)

[Jeff Law]

On 12/02/2015 12:16 AM, Jakub Jelinek wrote:
> On Tue, Dec 01, 2015 at 08:57:39PM -0700, Jeff Law wrote:
>> On 12/01/2015 01:34 PM, Jakub Jelinek wrote:
>>> Hi!
>>>
>>> get_parm_info right now uses input_location as the diagnostics locus, but as
>>> can be seen on the testcase, that is pretty random location at that point,
>>> often the type of the last parameter.
>>>
>>> This patch changes it to use the locus from the binding info.
>>>
>>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>>>
>>> 2015-12-01  Jakub Jelinek  <jakub@redhat.com>
>>>
>>> 	PR c/68533
>>> 	* c-decl.c (get_parm_info): Use b->locus instead of input_location
>>> 	for diagnostics.
>>>
>>> 	* gcc.dg/pr68533.c: New test.
>> I think the change itself is fine.  My question is whether or not the C++
>> front-end gets this right.  ISTM we ought to be running the test on both the
>> C & C++ front-ends.  The C++ front-end may emit different messages, but we
>> ought to be able to account for that and ensure that we're getting them on
>> the right lines.
>
> The warning does not exist at all in the C++ FE, it has instead
> just an error on declaring anonymous types (not others) among parameters.
> C++ does not have forward parameter declarations.  And for the void
> among arguments diagnostics it uses slightly different locations and
> completely different wording.  So I'm afraid there is basically nothing in
> common between C and C++ FEs in this area, so a shared testcase does not
> make sense.
In that case, it's fine as-is for the trunk.

Thanks for checking on the C++ side.

Jeff