[Patch to gcc/function] PR 58362

[Patch to gcc/function] PR 58362

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 427 bytes --]

Hi,

this patchlet fixes the column # of the unused parameter warnings 
emitted by do_warn_unused_parameter by explicitly passing 
DECL_SOURCE_LOCATION (decl) instead of wrongly relying on '+', which in 
this case ends up meaning the location of the function declaration. 
Tested x86_64-linux.

Thanks!
Paolo.

PS: sorry, not 100% sure about the maintainers of this file, I'm adding 
a couple of CCs.

////////////////////////

[-- Attachment #2: CL_58362 --]
[-- Type: text/plain, Size: 339 bytes --]

2013-09-09  Marc Glisse  <marc.glisse@inria.fr>
	    Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/58362
	* function.c (do_warn_unused_parameter): Use DECL_SOURCE_LOCATION.

/testsuite
2013-09-09  Marc Glisse  <marc.glisse@inria.fr>
	    Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/58362
	* c-c++-common/Wunused-parm-1.c: New.

[-- Attachment #3: patch_58362 --]
[-- Type: text/plain, Size: 988 bytes --]

Index: function.c
===================================================================
--- function.c	(revision 202365)
+++ function.c	(working copy)
@@ -5004,7 +5004,8 @@ do_warn_unused_parameter (tree fn)
     if (!TREE_USED (decl) && TREE_CODE (decl) == PARM_DECL
 	&& DECL_NAME (decl) && !DECL_ARTIFICIAL (decl)
 	&& !TREE_NO_WARNING (decl))
-      warning (OPT_Wunused_parameter, "unused parameter %q+D", decl);
+      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wunused_parameter,
+		  "unused parameter %qD", decl);
 }
 
 /* Generate RTL for the end of the current function.  */
Index: testsuite/c-c++-common/Wunused-parm-1.c
===================================================================
--- testsuite/c-c++-common/Wunused-parm-1.c	(revision 0)
+++ testsuite/c-c++-common/Wunused-parm-1.c	(working copy)
@@ -0,0 +1,5 @@
+/* PR c++/58362 */
+/* { dg-options "-Wunused-parameter" } */
+/* { dg-do compile } */
+
+void f (long s) { }  /* { dg-warning "14:unused parameter" } */

Re: [Patch to gcc/function] PR 58362

[Richard Biener]

On Sun, 8 Sep 2013, Paolo Carlini wrote:

> Hi,
> 
> this patchlet fixes the column # of the unused parameter warnings emitted by
> do_warn_unused_parameter by explicitly passing DECL_SOURCE_LOCATION (decl)
> instead of wrongly relying on '+', which in this case ends up meaning the
> location of the function declaration. Tested x86_64-linux.

I would have expected %q+D to use the location of the corresponding
decl, not some random other location.  So, isn't the bug in the
C++ frontend diagnostic machinery?

Thanks,
Richard.

Re: [Patch to gcc/function] PR 58362

[Paolo Carlini]

Hi Richard,

On 09/09/2013 09:46 AM, Richard Biener wrote:
> On Sun, 8 Sep 2013, Paolo Carlini wrote:
>
>> Hi,
>>
>> this patchlet fixes the column # of the unused parameter warnings emitted by
>> do_warn_unused_parameter by explicitly passing DECL_SOURCE_LOCATION (decl)
>> instead of wrongly relying on '+', which in this case ends up meaning the
>> location of the function declaration. Tested x86_64-linux.
> I would have expected %q+D to use the location of the corresponding
> decl, not some random other location.  So, isn't the bug in the
> C++ frontend diagnostic machinery?
Well, first notice that the patch fixes the issue *both* for the C and 
C++ front-ends, that's why I added the testcase to c-c++-common. This 
isn't a C++ issue. Then notice that we do already have tens of cases 
where we use DECL_SOURCE_LOCATION + %qD, when we want to be precise 
about the location. The diagnostic machinery has this mechanism using + 
which uses location_of, which is often useful for expressions, but which 
very often we don't use for decls. In fact, some people, like Manuel, 
see the audit trail of the bug, find the mechanism quite confusing. Is 
there something specific you want me to check?

Thanks,
Paolo.

Re: [Patch to gcc/function] PR 58362

[Richard Biener]

On Mon, 9 Sep 2013, Paolo Carlini wrote:

> Hi Richard,
> 
> On 09/09/2013 09:46 AM, Richard Biener wrote:
> > On Sun, 8 Sep 2013, Paolo Carlini wrote:
> > 
> > > Hi,
> > > 
> > > this patchlet fixes the column # of the unused parameter warnings emitted
> > > by
> > > do_warn_unused_parameter by explicitly passing DECL_SOURCE_LOCATION (decl)
> > > instead of wrongly relying on '+', which in this case ends up meaning the
> > > location of the function declaration. Tested x86_64-linux.
> > I would have expected %q+D to use the location of the corresponding
> > decl, not some random other location.  So, isn't the bug in the
> > C++ frontend diagnostic machinery?
> Well, first notice that the patch fixes the issue *both* for the C and C++
> front-ends, that's why I added the testcase to c-c++-common. This isn't a C++
> issue. Then notice that we do already have tens of cases where we use
> DECL_SOURCE_LOCATION + %qD, when we want to be precise about the location. The
> diagnostic machinery has this mechanism using + which uses location_of, which
> is often useful for expressions, but which very often we don't use for decls.
> In fact, some people, like Manuel, see the audit trail of the bug, find the
> mechanism quite confusing. Is there something specific you want me to check?

How is '+' in %q+D defined?  I failed to find documentation of the
diagnostic formats (but only searched for like two minutes).

Richard.

Re: [Patch to gcc/function] PR 58362

[Richard Biener]

On Mon, 9 Sep 2013, Richard Biener wrote:

> On Mon, 9 Sep 2013, Paolo Carlini wrote:
> 
> > Hi Richard,
> > 
> > On 09/09/2013 09:46 AM, Richard Biener wrote:
> > > On Sun, 8 Sep 2013, Paolo Carlini wrote:
> > > 
> > > > Hi,
> > > > 
> > > > this patchlet fixes the column # of the unused parameter warnings emitted
> > > > by
> > > > do_warn_unused_parameter by explicitly passing DECL_SOURCE_LOCATION (decl)
> > > > instead of wrongly relying on '+', which in this case ends up meaning the
> > > > location of the function declaration. Tested x86_64-linux.
> > > I would have expected %q+D to use the location of the corresponding
> > > decl, not some random other location.  So, isn't the bug in the
> > > C++ frontend diagnostic machinery?
> > Well, first notice that the patch fixes the issue *both* for the C and C++
> > front-ends, that's why I added the testcase to c-c++-common. This isn't a C++
> > issue. Then notice that we do already have tens of cases where we use
> > DECL_SOURCE_LOCATION + %qD, when we want to be precise about the location. The
> > diagnostic machinery has this mechanism using + which uses location_of, which
> > is often useful for expressions, but which very often we don't use for decls.
> > In fact, some people, like Manuel, see the audit trail of the bug, find the
> > mechanism quite confusing. Is there something specific you want me to check?
> 
> How is '+' in %q+D defined?  I failed to find documentation of the
> diagnostic formats (but only searched for like two minutes).

That said, grepping for %q+D reveals quite some uses and it looks like
all of them expect the location being used to be that of the decl passed
to the diagnostic call, not some random other location.

Richard.

Re: [Patch to gcc/function] PR 58362

[Jakub Jelinek]

On Mon, Sep 09, 2013 at 11:37:31AM +0200, Richard Biener wrote:
> > > > > this patchlet fixes the column # of the unused parameter warnings emitted
> > > > > by
> > > > > do_warn_unused_parameter by explicitly passing DECL_SOURCE_LOCATION (decl)
> > > > > instead of wrongly relying on '+', which in this case ends up meaning the
> > > > > location of the function declaration. Tested x86_64-linux.
> > > > I would have expected %q+D to use the location of the corresponding
> > > > decl, not some random other location.  So, isn't the bug in the
> > > > C++ frontend diagnostic machinery?
> > > Well, first notice that the patch fixes the issue *both* for the C and C++
> > > front-ends, that's why I added the testcase to c-c++-common. This isn't a C++
> > > issue. Then notice that we do already have tens of cases where we use
> > > DECL_SOURCE_LOCATION + %qD, when we want to be precise about the location. The
> > > diagnostic machinery has this mechanism using + which uses location_of, which
> > > is often useful for expressions, but which very often we don't use for decls.
> > > In fact, some people, like Manuel, see the audit trail of the bug, find the
> > > mechanism quite confusing. Is there something specific you want me to check?
> > 
> > How is '+' in %q+D defined?  I failed to find documentation of the
> > diagnostic formats (but only searched for like two minutes).
> 
> That said, grepping for %q+D reveals quite some uses and it looks like
> all of them expect the location being used to be that of the decl passed
> to the diagnostic call, not some random other location.

The C++ FE locus handling is in very bad shape (C FE is much better, Aldy
and others have spent quite some time fixing all the issues).  I guess this
is just one of the many issues.  The most annoying to me is that the C++ FE
for function calls uses the location of the closing ) of the call expression
rather than the function name or at least opening (, so if you have a 
  call_something (one_arg,
		  second_arg,
		  third_arg,
		  fourth_arg,
		  fifth_arg);
you really don't see what is being called in the debugger when debugging
C++.

	Jakub

Re: [Patch to gcc/function] PR 58362

[Richard Biener]

On Mon, 9 Sep 2013, Jakub Jelinek wrote:

> On Mon, Sep 09, 2013 at 11:37:31AM +0200, Richard Biener wrote:
> > > > > > this patchlet fixes the column # of the unused parameter warnings emitted
> > > > > > by
> > > > > > do_warn_unused_parameter by explicitly passing DECL_SOURCE_LOCATION (decl)
> > > > > > instead of wrongly relying on '+', which in this case ends up meaning the
> > > > > > location of the function declaration. Tested x86_64-linux.
> > > > > I would have expected %q+D to use the location of the corresponding
> > > > > decl, not some random other location.  So, isn't the bug in the
> > > > > C++ frontend diagnostic machinery?
> > > > Well, first notice that the patch fixes the issue *both* for the C and C++
> > > > front-ends, that's why I added the testcase to c-c++-common. This isn't a C++
> > > > issue. Then notice that we do already have tens of cases where we use
> > > > DECL_SOURCE_LOCATION + %qD, when we want to be precise about the location. The
> > > > diagnostic machinery has this mechanism using + which uses location_of, which
> > > > is often useful for expressions, but which very often we don't use for decls.
> > > > In fact, some people, like Manuel, see the audit trail of the bug, find the
> > > > mechanism quite confusing. Is there something specific you want me to check?
> > > 
> > > How is '+' in %q+D defined?  I failed to find documentation of the
> > > diagnostic formats (but only searched for like two minutes).
> > 
> > That said, grepping for %q+D reveals quite some uses and it looks like
> > all of them expect the location being used to be that of the decl passed
> > to the diagnostic call, not some random other location.
> 
> The C++ FE locus handling is in very bad shape (C FE is much better, Aldy
> and others have spent quite some time fixing all the issues).  I guess this
> is just one of the many issues. 

Well, in this case the patch should IMHO be a no-op.

-      warning (OPT_Wunused_parameter, "unused parameter %q+D", decl);
+      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wunused_parameter,
+		  "unused parameter %qD", decl);

no?  Unless I misunderstand what %q+D should do.

Richard.

Re: [Patch to gcc/function] PR 58362

[Jakub Jelinek]

On Mon, Sep 09, 2013 at 11:45:08AM +0200, Richard Biener wrote:
> Well, in this case the patch should IMHO be a no-op.
> 
> -      warning (OPT_Wunused_parameter, "unused parameter %q+D", decl);
> +      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wunused_parameter,
> +		  "unused parameter %qD", decl);
> 
> no?  Unless I misunderstand what %q+D should do.

The question is how exactly is %q+D defined, if it is
warning_at (location_of (decl), OPT_Wunused_parameter, "unused parameter %qD", decl); in this case, or 
DECL_SOURCE_LOCATION (decl) instead.

	Jakub

Re: [Patch to gcc/function] PR 58362

[Richard Biener]

On Mon, 9 Sep 2013, Jakub Jelinek wrote:

> On Mon, Sep 09, 2013 at 11:45:08AM +0200, Richard Biener wrote:
> > Well, in this case the patch should IMHO be a no-op.
> > 
> > -      warning (OPT_Wunused_parameter, "unused parameter %q+D", decl);
> > +      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wunused_parameter,
> > +		  "unused parameter %qD", decl);
> > 
> > no?  Unless I misunderstand what %q+D should do.
> 
> The question is how exactly is %q+D defined, if it is
> warning_at (location_of (decl), OPT_Wunused_parameter, "unused parameter %qD", decl); in this case, or 
> DECL_SOURCE_LOCATION (decl) instead.

It can't be 'location_of' because that's a C++ FE speciality but
warning_at and %q+D are diagnostic machinery level.

Unless of course the meaning of %q+D depends on the frontend which
would make its use from the middle-end ill-defined.

Richard.

Re: [Patch to gcc/function] PR 58362

[Paolo Carlini]

On 09/09/2013 12:04 PM, Richard Biener wrote:
> On Mon, 9 Sep 2013, Jakub Jelinek wrote:
>
>> On Mon, Sep 09, 2013 at 11:45:08AM +0200, Richard Biener wrote:
>>> Well, in this case the patch should IMHO be a no-op.
>>>
>>> -      warning (OPT_Wunused_parameter, "unused parameter %q+D", decl);
>>> +      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wunused_parameter,
>>> +		  "unused parameter %qD", decl);
>>>
>>> no?  Unless I misunderstand what %q+D should do.
>> The question is how exactly is %q+D defined, if it is
>> warning_at (location_of (decl), OPT_Wunused_parameter, "unused parameter %qD", decl); in this case, or
>> DECL_SOURCE_LOCATION (decl) instead.
> It can't be 'location_of' because that's a C++ FE speciality but
> warning_at and %q+D are diagnostic machinery level.
Everything happens via call backs. Thus from the generic diagnostic 
machinery, you go to cp_printer for C++, thus location_of for C++. In C 
is different, but again there is, evidently, a mechanism which uses 
DECL_CONTEXT for PARM_DECLs which leads to an inaccurate location when 
we *really* want the location of the parameter (exactly as I explained 
for C++).

Paolo.

Re: [Patch to gcc/function] PR 58362

[Paolo Carlini]

On 09/09/2013 12:08 PM, Paolo Carlini wrote:
> Everything happens via call backs. Thus from the generic diagnostic 
> machinery, you go to cp_printer for C++, thus location_of for C++. In 
> C is different, but again there is, evidently, a mechanism which uses 
> DECL_CONTEXT for PARM_DECLs which leads to an inaccurate location when 
> we *really* want the location of the parameter (exactly as I explained 
> for C++).
See line # 615 of pretty-print.c, that call leads you back to the 
front-end, thus cp_printer etc, for C++

Paolo.

Re: [Patch to gcc/function] PR 58362

[Richard Biener]

On Mon, 9 Sep 2013, Paolo Carlini wrote:

> On 09/09/2013 12:04 PM, Richard Biener wrote:
> > On Mon, 9 Sep 2013, Jakub Jelinek wrote:
> > 
> > > On Mon, Sep 09, 2013 at 11:45:08AM +0200, Richard Biener wrote:
> > > > Well, in this case the patch should IMHO be a no-op.
> > > > 
> > > > -      warning (OPT_Wunused_parameter, "unused parameter %q+D", decl);
> > > > +      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wunused_parameter,
> > > > +		  "unused parameter %qD", decl);
> > > > 
> > > > no?  Unless I misunderstand what %q+D should do.
> > > The question is how exactly is %q+D defined, if it is
> > > warning_at (location_of (decl), OPT_Wunused_parameter, "unused parameter
> > > %qD", decl); in this case, or
> > > DECL_SOURCE_LOCATION (decl) instead.
> > It can't be 'location_of' because that's a C++ FE speciality but
> > warning_at and %q+D are diagnostic machinery level.
> Everything happens via call backs. Thus from the generic diagnostic machinery,
> you go to cp_printer for C++, thus location_of for C++. In C is different, but
> again there is, evidently, a mechanism which uses DECL_CONTEXT for PARM_DECLs
> which leads to an inaccurate location when we *really* want the location of
> the parameter (exactly as I explained for C++).

I understand that.  But I question it.  Why would that ever be useful?
Can't the places that want that simply use warning/error_at with the
proper location?

Richard.

Re: [Patch to gcc/function] PR 58362

[Paolo Carlini]

Hi,

On 09/09/2013 12:13 PM, Richard Biener wrote:
>> Everything happens via call backs. Thus from the generic diagnostic machinery,
>> you go to cp_printer for C++, thus location_of for C++. In C is different, but
>> again there is, evidently, a mechanism which uses DECL_CONTEXT for PARM_DECLs
>> which leads to an inaccurate location when we *really* want the location of
>> the parameter (exactly as I explained for C++).
> I understand that.  But I question it.  Why would that ever be useful?
> Can't the places that want that simply use warning/error_at with the
> proper location?
Indeed, this is *exactly*, if I understand him correctly, what Manuel 
says: looking forward he thinks we should not have this magic relying on 
'+' and things happening behind the scenes, simply explicit locations 
and warning_at/error_at.

For the time being, however, we are stuck with the situation that 
relying on '+' in this case (like in *many* existing others) doesn't 
work, because, in C++, location_of does t = DECL_CONTEXT (t) which is 
almost right but not really, we want the location of the PARM_DECL. The 
C front-end does something very similar (as the inaccurate location shows).

What do you think?

Paolo.

Re: [Patch to gcc/function] PR 58362

[Gabriel Dos Reis]

On Mon, Sep 9, 2013 at 5:13 AM, Richard Biener <rguenther@suse.de> wrote:
> On Mon, 9 Sep 2013, Paolo Carlini wrote:
>
>> On 09/09/2013 12:04 PM, Richard Biener wrote:
>> > On Mon, 9 Sep 2013, Jakub Jelinek wrote:
>> >
>> > > On Mon, Sep 09, 2013 at 11:45:08AM +0200, Richard Biener wrote:
>> > > > Well, in this case the patch should IMHO be a no-op.
>> > > >
>> > > > -      warning (OPT_Wunused_parameter, "unused parameter %q+D", decl);
>> > > > +      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wunused_parameter,
>> > > > +                 "unused parameter %qD", decl);
>> > > >
>> > > > no?  Unless I misunderstand what %q+D should do.
>> > > The question is how exactly is %q+D defined, if it is
>> > > warning_at (location_of (decl), OPT_Wunused_parameter, "unused parameter
>> > > %qD", decl); in this case, or
>> > > DECL_SOURCE_LOCATION (decl) instead.
>> > It can't be 'location_of' because that's a C++ FE speciality but
>> > warning_at and %q+D are diagnostic machinery level.
>> Everything happens via call backs. Thus from the generic diagnostic machinery,
>> you go to cp_printer for C++, thus location_of for C++. In C is different, but
>> again there is, evidently, a mechanism which uses DECL_CONTEXT for PARM_DECLs
>> which leads to an inaccurate location when we *really* want the location of
>> the parameter (exactly as I explained for C++).
>
> I understand that.  But I question it.  Why would that ever be useful?
> Can't the places that want that simply use warning/error_at with the
> proper location?

I agree with Richard that if you want a locus that is not the current locus,
and is not the locus of the decl, you should use the _at version.

-- Gaby

Re: [Patch to gcc/function] PR 58362

[Gabriel Dos Reis]

On Mon, Sep 9, 2013 at 5:04 AM, Richard Biener <rguenther@suse.de> wrote:
> On Mon, 9 Sep 2013, Jakub Jelinek wrote:
>
>> On Mon, Sep 09, 2013 at 11:45:08AM +0200, Richard Biener wrote:
>> > Well, in this case the patch should IMHO be a no-op.
>> >
>> > -      warning (OPT_Wunused_parameter, "unused parameter %q+D", decl);
>> > +      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wunused_parameter,
>> > +             "unused parameter %qD", decl);
>> >
>> > no?  Unless I misunderstand what %q+D should do.
>>
>> The question is how exactly is %q+D defined, if it is
>> warning_at (location_of (decl), OPT_Wunused_parameter, "unused parameter %qD", decl); in this case, or
>> DECL_SOURCE_LOCATION (decl) instead.
>
> It can't be 'location_of' because that's a C++ FE speciality but
> warning_at and %q+D are diagnostic machinery level.

%+D was, and has for long, been a C++ FE-specific marker.

I don't recall when we decided to make that available to all front-ends.

> Unless of course the meaning of %q+D depends on the frontend which
> would make its use from the middle-end ill-defined.

We introduced xxx_at so that a different locus (different from the
decl and different from current locus, if ever defined) can be specified.

-- Gaby

Re: [Patch to gcc/function] PR 58362

[Gabriel Dos Reis]

On Mon, Sep 9, 2013 at 4:46 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Sep 09, 2013 at 11:45:08AM +0200, Richard Biener wrote:
>> Well, in this case the patch should IMHO be a no-op.
>>
>> -      warning (OPT_Wunused_parameter, "unused parameter %q+D", decl);
>> +      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wunused_parameter,
>> +               "unused parameter %qD", decl);
>>
>> no?  Unless I misunderstand what %q+D should do.
>
> The question is how exactly is %q+D defined, if it is
> warning_at (location_of (decl), OPT_Wunused_parameter, "unused parameter %qD", decl); in this case, or
> DECL_SOURCE_LOCATION (decl) instead.

The semantics of '%+D' was defined long before I got involved.

The way it was supposed to work is that we pick the location
of the decl being specified, instead of taking the current
location.  When we figured that was insufficient, we introduced
%H to say: pick this location.  For that reason, one can only have
on +D in a diagnostic message (I don't think we

-- Gaby

Re: [Patch to gcc/function] PR 58362

[Paolo Carlini]

Hi,

On 09/09/2013 11:45 AM, Richard Biener wrote:
> Well, in this case the patch should IMHO be a no-op.
>
> -      warning (OPT_Wunused_parameter, "unused parameter %q+D", decl);
> +      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wunused_parameter,
> +		  "unused parameter %qD", decl);
>
> no?  Unless I misunderstand what %q+D should do.
See my previous email Richard. The situation should be rather clear now.

Paolo.

Re: [Patch to gcc/function] PR 58362

[Paolo Carlini]

On 09/09/2013 11:37 AM, Richard Biener wrote:
> That said, grepping for %q+D reveals quite some uses and it looks like
> all of them expect the location being used to be that of the decl passed
> to the diagnostic call, not some random other location.
If the decl is *not* a PARM_DECL, I expect %q+D to be often accurate. In 
fact, even when *is* a PARM_DECL what we have now is pretty decent, 
because normally the location of the corresponding FUNCTION_DECL isn't 
that far. The point is whether we want to be *more* accurate and point 
to the specific unused parameter, for C and C++, as clang and icc do.

Paolo.

Re: [Patch to gcc/function] PR 58362

[Jakub Jelinek]

On Mon, Sep 09, 2013 at 12:38:46PM +0200, Paolo Carlini wrote:
> On 09/09/2013 11:37 AM, Richard Biener wrote:
> >That said, grepping for %q+D reveals quite some uses and it looks like
> >all of them expect the location being used to be that of the decl passed
> >to the diagnostic call, not some random other location.
> If the decl is *not* a PARM_DECL, I expect %q+D to be often
> accurate. In fact, even when *is* a PARM_DECL what we have now is
> pretty decent, because normally the location of the corresponding
> FUNCTION_DECL isn't that far. The point is whether we want to be
> *more* accurate and point to the specific unused parameter, for C
> and C++, as clang and icc do.

I guess the primary question is why location_of special cases the PARM_DECL
and in which case it is useful to do so, and whether the number of cases (if
any) when it is useful to do so is bigger than the number of place when it
is undesirable.

	Jakub

Re: [Patch to gcc/function] PR 58362

[Paolo Carlini]

On 09/09/2013 12:41 PM, Jakub Jelinek wrote:
> On Mon, Sep 09, 2013 at 12:38:46PM +0200, Paolo Carlini wrote:
>> On 09/09/2013 11:37 AM, Richard Biener wrote:
>>> That said, grepping for %q+D reveals quite some uses and it looks like
>>> all of them expect the location being used to be that of the decl passed
>>> to the diagnostic call, not some random other location.
>> If the decl is *not* a PARM_DECL, I expect %q+D to be often
>> accurate. In fact, even when *is* a PARM_DECL what we have now is
>> pretty decent, because normally the location of the corresponding
>> FUNCTION_DECL isn't that far. The point is whether we want to be
>> *more* accurate and point to the specific unused parameter, for C
>> and C++, as clang and icc do.
> I guess the primary question is why location_of special cases the PARM_DECL
> and in which case it is useful to do so, and whether the number of cases (if
> any) when it is useful to do so is bigger than the number of place when it
> is undesirable.
I understand that. It seems to me a much bigger project and must be done 
for the C front-end too (I don't know the name of the equivalent of 
location_of, but the location is wrong for it too, there must be the 
equivalent of t = DECL_CONTEXT (t) for it too)

What can I tell you, I *may* be able to work on that, but not now, not 
for both front-ends.

Paolo.

Re: [Patch to gcc/function] PR 58362

[Paolo Carlini]

On 09/09/2013 12:44 PM, Paolo Carlini wrote:
> I understand that. It seems to me a much bigger project and must be 
> done for the C front-end too (I don't know the name of the equivalent 
> of location_of, but the location is wrong for it too, there must be 
> the equivalent of t = DECL_CONTEXT (t) for it too)
Sorry, I stand corrected, the C front-end is fine (yesterday I worked on 
other diagnostic issues impacting both). That changes the issue 
completely. Of course we want to only change location_of.

Paolo.

Re: [Patch to gcc/function] PR 58362

[Gabriel Dos Reis]

On Mon, Sep 9, 2013 at 5:41 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Sep 09, 2013 at 12:38:46PM +0200, Paolo Carlini wrote:
>> On 09/09/2013 11:37 AM, Richard Biener wrote:
>> >That said, grepping for %q+D reveals quite some uses and it looks like
>> >all of them expect the location being used to be that of the decl passed
>> >to the diagnostic call, not some random other location.
>> If the decl is *not* a PARM_DECL, I expect %q+D to be often
>> accurate. In fact, even when *is* a PARM_DECL what we have now is
>> pretty decent, because normally the location of the corresponding
>> FUNCTION_DECL isn't that far. The point is whether we want to be
>> *more* accurate and point to the specific unused parameter, for C
>> and C++, as clang and icc do.
>
> I guess the primary question is why location_of special cases the PARM_DECL
> and in which case it is useful to do so, and whether the number of cases (if
> any) when it is useful to do so is bigger than the number of place when it
> is undesirable.

Most likely historical reason, the exact sequence of which is lost to history.

-- Gaby

Re: [Patch to gcc/function] PR 58362

[Gabriel Dos Reis]

On Mon, Sep 9, 2013 at 5:38 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> On 09/09/2013 11:37 AM, Richard Biener wrote:
>>
>> That said, grepping for %q+D reveals quite some uses and it looks like
>> all of them expect the location being used to be that of the decl passed
>> to the diagnostic call, not some random other location.
>
> If the decl is *not* a PARM_DECL, I expect %q+D to be often accurate. In
> fact, even when *is* a PARM_DECL what we have now is pretty decent, because
> normally the location of the corresponding FUNCTION_DECL isn't that far. The
> point is whether we want to be *more* accurate and point to the specific
> unused parameter, for C and C++, as clang and icc do.

I think the logic is simpler if we use the xxx_at form in these cases.

-- Gaby

Re: [Patch to gcc/function] PR 58362

[Paolo Carlini]

Hi,

On 09/09/2013 11:33 AM, Richard Biener wrote:
> How is '+' in %q+D defined? I failed to find documentation of the 
> diagnostic formats (but only searched for like two minutes). 
You see, this is an issue ;) No seriously, in C++ I understand it by 
looking at error.c:3438: when '+' is there, cp_printer use location_of 
(t) instead of what is passed down in error_at or warning_at, etc. Then 
if you go to location_of, in the same file, you see the reason of the 
difference between DECL_SOURCE_LOCATION + "%qD" and "%q+D":

   if (TREE_CODE (t) == PARM_DECL && DECL_CONTEXT (t))
     t = DECL_CONTEXT (t);
   else if ...

thus, in case of PARM_DECLs, t becomes DECL_CONTEXT (t), thus the 
function declaration instead of the parameter declaration!!! See? (I'm 
learning the details with you)

Now, I think we have two options: either we do more or less what Marc & 
I & Manuel figured out, or we try to change the definition of 
location_of, which impacts a lot of code...

Paolo.