Re: C PATCH to display types when printing a conversion warning (PR c/81233)

[Marek Polacek]

On Thu, Aug 10, 2017 at 10:52:54AM +0200, Andreas Schwab wrote:
> On Jul 13 2017, Marek Polacek  wrote:
> 
> > diff --git gcc/testsuite/objc.dg/proto-lossage-4.m 
> > gcc/testsuite/objc.dg/proto-lossage-4.m
> > index e72328b3703..4c6b560bab4 100644
> > --- gcc/testsuite/objc.dg/proto-lossage-4.m
> > +++ gcc/testsuite/objc.dg/proto-lossage-4.m
> > @@ -28,13 +28,13 @@ long foo(void) {
> >receiver += [receiver anotherValue]; /* { dg-warning "invalid receiver 
> > type .intptr_t." } */
> >  
> >receiver += [(Obj *)receiver someValue]; /* { dg-warning ".Obj. may not 
> > respond to .\\-someValue." } */
> > -/* { dg-warning "assignment makes integer from pointer without a cast" "" 
> > { target *-*-* } .-1 } */
> > +/* { dg-warning "assignment to 'intptr_t {aka long int}' from 'id' makes 
> > integer from pointer without a cast" "" { target *-*-* } .-1 } */
> >  
> >receiver += [(Obj *)receiver anotherValue];
> >receiver += [(Obj  *)receiver someValue];
> >receiver += [(Obj  *)receiver anotherValue];
> >receiver += [objrcvr someValue]; /* { dg-warning ".Obj. may not respond 
> > to .\\-someValue." } */
> > -/* { dg-warning "assignment makes integer from pointer without a cast" "" 
> > { target *-*-* } .-1 } */
> > +/* { dg-warning "assignment to 'intptr_t {aka long int}' from 'id' makes 
> > integer from pointer without a cast" "" { target *-*-* } .-1 } */
> >  
> >receiver += [objrcvr anotherValue];
> >receiver += [(Obj  *)objrcvr someValue];
> > @@ -42,7 +42,7 @@ long foo(void) {
> >receiver += [objrcvr2 someValue];
> >receiver += [objrcvr2 anotherValue];
> >receiver += [(Obj *)objrcvr2 someValue]; /* { dg-warning ".Obj. may not 
> > respond to .\\-someValue." } */
> > -/* { dg-warning "assignment makes integer from pointer without a cast" "" 
> > { target *-*-* } .-1 } */
> > +/* { dg-warning "assignment to 'intptr_t {aka long int}' from 'id' makes 
> > integer from pointer without a cast" "" { target *-*-* } .-1 } */
> >  
> >receiver += [(Obj *)objrcvr2 anotherValue];
> >  
> 
> FAIL: objc.dg/proto-lossage-4.m -fgnu-runtime  (test for warnings, line 30)
> FAIL: objc.dg/proto-lossage-4.m -fgnu-runtime  (test for warnings, line 36)
> FAIL: objc.dg/proto-lossage-4.m -fgnu-runtime  (test for warnings, line 44)
> FAIL: objc.dg/proto-lossage-4.m -fgnu-runtime (test for excess errors)
> Excess errors:
> /daten/aranym/gcc/gcc-20170810/gcc/testsuite/objc.dg/proto-lossage-4.m:30:12: 
> warning: assignment to 'intptr_t {aka int}' from 'id' makes integer from 
> pointer without a cast [-Wint-conversion]
> /daten/aranym/gcc/gcc-20170810/gcc/testsuite/objc.dg/proto-lossage-4.m:36:12: 
> warning: assignment to 'intptr_t {aka int}' from 'id' makes integer from 
> pointer without a cast [-Wint-conversion]
> /daten/aranym/gcc/gcc-20170810/gcc/testsuite/objc.dg/proto-lossage-4.m:44:12: 
> warning: assignment to 'intptr_t {aka int}' from 'id' makes integer from 
> pointer without a cast [-Wint-conversion]

Argh.  Sorry, will fix now.

Marek

Re: C PATCH to display types when printing a conversion warning (PR c/81233)

[Andreas Schwab]

On Jul 13 2017, Marek Polacek  wrote:

> diff --git gcc/testsuite/objc.dg/proto-lossage-4.m 
> gcc/testsuite/objc.dg/proto-lossage-4.m
> index e72328b3703..4c6b560bab4 100644
> --- gcc/testsuite/objc.dg/proto-lossage-4.m
> +++ gcc/testsuite/objc.dg/proto-lossage-4.m
> @@ -28,13 +28,13 @@ long foo(void) {
>receiver += [receiver anotherValue]; /* { dg-warning "invalid receiver 
> type .intptr_t." } */
>  
>receiver += [(Obj *)receiver someValue]; /* { dg-warning ".Obj. may not 
> respond to .\\-someValue." } */
> -/* { dg-warning "assignment makes integer from pointer without a cast" "" { 
> target *-*-* } .-1 } */
> +/* { dg-warning "assignment to 'intptr_t {aka long int}' from 'id' makes 
> integer from pointer without a cast" "" { target *-*-* } .-1 } */
>  
>receiver += [(Obj *)receiver anotherValue];
>receiver += [(Obj  *)receiver someValue];
>receiver += [(Obj  *)receiver anotherValue];
>receiver += [objrcvr someValue]; /* { dg-warning ".Obj. may not respond to 
> .\\-someValue." } */
> -/* { dg-warning "assignment makes integer from pointer without a cast" "" { 
> target *-*-* } .-1 } */
> +/* { dg-warning "assignment to 'intptr_t {aka long int}' from 'id' makes 
> integer from pointer without a cast" "" { target *-*-* } .-1 } */
>  
>receiver += [objrcvr anotherValue];
>receiver += [(Obj  *)objrcvr someValue];
> @@ -42,7 +42,7 @@ long foo(void) {
>receiver += [objrcvr2 someValue];
>receiver += [objrcvr2 anotherValue];
>receiver += [(Obj *)objrcvr2 someValue]; /* { dg-warning ".Obj. may not 
> respond to .\\-someValue." } */
> -/* { dg-warning "assignment makes integer from pointer without a cast" "" { 
> target *-*-* } .-1 } */
> +/* { dg-warning "assignment to 'intptr_t {aka long int}' from 'id' makes 
> integer from pointer without a cast" "" { target *-*-* } .-1 } */
>  
>receiver += [(Obj *)objrcvr2 anotherValue];
>  

FAIL: objc.dg/proto-lossage-4.m -fgnu-runtime  (test for warnings, line 30)
FAIL: objc.dg/proto-lossage-4.m -fgnu-runtime  (test for warnings, line 36)
FAIL: objc.dg/proto-lossage-4.m -fgnu-runtime  (test for warnings, line 44)
FAIL: objc.dg/proto-lossage-4.m -fgnu-runtime (test for excess errors)
Excess errors:
/daten/aranym/gcc/gcc-20170810/gcc/testsuite/objc.dg/proto-lossage-4.m:30:12: 
warning: assignment to 'intptr_t {aka int}' from 'id' makes integer from 
pointer without a cast [-Wint-conversion]
/daten/aranym/gcc/gcc-20170810/gcc/testsuite/objc.dg/proto-lossage-4.m:36:12: 
warning: assignment to 'intptr_t {aka int}' from 'id' makes integer from 
pointer without a cast [-Wint-conversion]
/daten/aranym/gcc/gcc-20170810/gcc/testsuite/objc.dg/proto-lossage-4.m:44:12: 
warning: assignment to 'intptr_t {aka int}' from 'id' makes integer from 
pointer without a cast [-Wint-conversion]

Andreas.

-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: C PATCH to display types when printing a conversion warning (PR c/81233)

[Joseph Myers]

On Thu, 13 Jul 2017, Marek Polacek wrote:

> Bootstrapped/regtested on x86_64-linux and powerpc64le-unknown-linux-gnu,
> ok for trunk?

OK.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: C PATCH to display types when printing a conversion warning (PR c/81233)

[Marek Polacek]

Ping.

On Mon, Jul 31, 2017 at 01:27:01PM +0200, Marek Polacek wrote:
> Ping.
> 
> On Thu, Jul 20, 2017 at 12:53:10PM +0200, Marek Polacek wrote:
> > On Wed, Jul 19, 2017 at 10:51:33AM -0400, David Malcolm wrote:
> > > The changes to diagnostic-core.h and diagnostic.c are OK.
> > 
> > Thanks.
> >  
> > > > Also,
> > > > PEDWARN_FOR_ASSIGNMENT didn't work with the addition of printing TYPE
> > > > and
> > > > RHSTYPE so I just decided to unroll the macro instead of making it
> > > > even more
> > > > ugly.
> > > > This patch is long but it's mainly because of the testsuite fallout.
> > > 
> > > The comment by PEDWARN_FOR_ASSIGNMENT says:
> > > 
> > > 
> > >   /* This macro is used to emit diagnostics to ensure that all format
> > >  strings are complete sentences, visible to gettext and checked
> > > at
> > >  compile time.  */
> > > 
> > > I wonder if it's possible to convert it to an inline function to get
> > > the same test coverage, without unrolling the macro?
> > 
> > Yeah, I tried, but the resulting inline function would have to have 12
> > parameters if I count well and that didn't seem like a win.  Perhaps
> > splitting convert_for_assignment would make sense, but likely not as
> > part of this patch.

Marek

Re: C PATCH to display types when printing a conversion warning (PR c/81233)

[Marek Polacek]

Ping.

On Thu, Jul 20, 2017 at 12:53:10PM +0200, Marek Polacek wrote:
> On Wed, Jul 19, 2017 at 10:51:33AM -0400, David Malcolm wrote:
> > The changes to diagnostic-core.h and diagnostic.c are OK.
> 
> Thanks.
>  
> > > Also,
> > > PEDWARN_FOR_ASSIGNMENT didn't work with the addition of printing TYPE
> > > and
> > > RHSTYPE so I just decided to unroll the macro instead of making it
> > > even more
> > > ugly.
> > > This patch is long but it's mainly because of the testsuite fallout.
> > 
> > The comment by PEDWARN_FOR_ASSIGNMENT says:
> > 
> > 
> >   /* This macro is used to emit diagnostics to ensure that all format
> >  strings are complete sentences, visible to gettext and checked
> > at
> >  compile time.  */
> > 
> > I wonder if it's possible to convert it to an inline function to get
> > the same test coverage, without unrolling the macro?
> 
> Yeah, I tried, but the resulting inline function would have to have 12
> parameters if I count well and that didn't seem like a win.  Perhaps
> splitting convert_for_assignment would make sense, but likely not as
> part of this patch.

Marek

Re: C PATCH to display types when printing a conversion warning (PR c/81233)

[Marek Polacek]

On Wed, Jul 19, 2017 at 10:51:33AM -0400, David Malcolm wrote:
> The changes to diagnostic-core.h and diagnostic.c are OK.

Thanks.
 
> > Also,
> > PEDWARN_FOR_ASSIGNMENT didn't work with the addition of printing TYPE
> > and
> > RHSTYPE so I just decided to unroll the macro instead of making it
> > even more
> > ugly.
> > This patch is long but it's mainly because of the testsuite fallout.
> 
> The comment by PEDWARN_FOR_ASSIGNMENT says:
> 
> 
>   /* This macro is used to emit diagnostics to ensure that all format
>  strings are complete sentences, visible to gettext and checked
> at
>  compile time.  */
> 
> I wonder if it's possible to convert it to an inline function to get
> the same test coverage, without unrolling the macro?

Yeah, I tried, but the resulting inline function would have to have 12
parameters if I count well and that didn't seem like a win.  Perhaps
splitting convert_for_assignment would make sense, but likely not as
part of this patch.

Marek

Re: C PATCH to display types when printing a conversion warning (PR c/81233)

[David Malcolm]

On Thu, 2017-07-13 at 16:18 +0200, Marek Polacek wrote:
> This patch improves diagnostic in the C FE by printing the types when
> reporting
> a problem with a conversion.  E.g., instead of 
> 
>warning: assignment from incompatible pointer type
> 
> you'll now get
> 
>   warning: assignment to 'int *' from incompatible pointer type 'char
> *'
> 
> or instead of
> 
>   warning: initialization makes integer from pointer without a cast
> 
> this
> 
>warning: initialization of 'int *' from 'int' makes pointer from
> integer without a cast
> 
> I've been wanting this for a long time and here it is.  Two snags: I
> had to
> make pedwarn_init to take '...' for which I had to introduce
> emit_diagnostic_valist; you can't pass varargs from one vararg
> function to
> another vararg function (and a macro with __VA_ARGS__ didn't work
> here).  

The changes to diagnostic-core.h and diagnostic.c are OK.

> Also,
> PEDWARN_FOR_ASSIGNMENT didn't work with the addition of printing TYPE
> and
> RHSTYPE so I just decided to unroll the macro instead of making it
> even more
> ugly.
> This patch is long but it's mainly because of the testsuite fallout.

The comment by PEDWARN_FOR_ASSIGNMENT says:


  /* This macro is used to emit diagnostics to ensure that all format
 strings are complete sentences, visible to gettext and checked
at
 compile time.  */

I wonder if it's possible to convert it to an inline function to get
the same test coverage, without unrolling the macro?

[...snip...]

Dave

Re: C PATCH to display types when printing a conversion warning (PR c/81233)

[Marek Polacek]

On Fri, Jul 14, 2017 at 02:02:34PM -0600, Martin Sebor wrote:
> Just to be clear: I don't mean to suggest to do this in this patch
> or necessarily even for this warning.  I'm not even sure to what
> extent it might be doable.  I mention it mostly as food for thought.

Sure, and it makes sense (where it's possible).  But this particular
patch just adds the types to the diagnostic and I think anything more
is beyond the scope of this patch.  So - still presenting it as it was :).

Thanks for looking into this,

Marek

Re: C PATCH to display types when printing a conversion warning (PR c/81233)

[Martin Sebor]

On 07/14/2017 09:40 AM, Martin Sebor wrote:

On 07/14/2017 07:47 AM, Marek Polacek wrote:

On Fri, Jul 14, 2017 at 02:52:36PM +0200, Marek Polacek wrote:

On Thu, Jul 13, 2017 at 11:42:15AM -0600, Martin Sebor wrote:

On 07/13/2017 08:18 AM, Marek Polacek wrote:

This patch improves diagnostic in the C FE by printing the types
when reporting
a problem with a conversion.  E.g., instead of

   warning: assignment from incompatible pointer type

you'll now get

  warning: assignment to 'int *' from incompatible pointer type
'char *'

or instead of

  warning: initialization makes integer from pointer without a cast

this

   warning: initialization of 'int *' from 'int' makes pointer from
integer without a cast

I've been wanting this for a long time and here it is.  Two snags:
I had to
make pedwarn_init to take '...' for which I had to introduce
emit_diagnostic_valist; you can't pass varargs from one vararg
function to
another vararg function (and a macro with __VA_ARGS__ didn't work
here).  Also,
PEDWARN_FOR_ASSIGNMENT didn't work with the addition of printing
TYPE and
RHSTYPE so I just decided to unroll the macro instead of making it
even more
ugly.  This patch is long but it's mainly because of the testsuite
fallout.

If you have better ideas about the wording, let me know.


It looks pretty good as is.  My only wording suggestion is to
consider simply mentioning conversion in the text of the warnings:

  warning: conversion to T* from an incompatible type U*

I'm not sure that being explicit about the context where the bad
conversion takes place (initialization vs assignment vs returning
a value) is terribly helpful.  That would not only simplify the
code and make all the messages consistent, but it would also make
it possible to get rid of the note when passing arguments.


Yeah, I agree, actually.  We print the expressions in question
(although,
we could probably do even better), and I don't see why it would be
necessary to mention whether it's an initialization or an assignment.
I think I'll just drop this patch and do something along the lines you
suggest.

David, do you agree with this?  (Joseph's on PTO but I'd of course like
to hear his opinion, too.)


I think I changed my mind.  Because clang says e.g.
warning: returning 'unsigned int *' from a function with result type
'int *'
  converts between pointers to integer types with different sign
so in the end it might be best to just go with my current patch; it
should
only improve things anyway.  And then add the fix-it hint.


Yes, Clang does do that.  G++, OTOH, prints an error with the same
text in all these cases.  It's not tremendously important which of
the two forms is used.  What I do think would be nice is if the text
of the same diagnostics, whether warnings or errors, could be more
consistent between the front ends.  A good way to do that in general,
if all of the checking code cannot be shared, is to provide a shared
diagnose_this_or_that() function for each diagnostic.  The function
would be parameterized on the kind of diagnostic (i.e., error or
warning), but would hardcode the shared text.  Each FE would call
it with an argument telling it whether to issue it as an error or
warning.


Just to be clear: I don't mean to suggest to do this in this patch
or necessarily even for this warning.  I'm not even sure to what
extent it might be doable.  I mention it mostly as food for thought.



Martin


One more thing: for

int *q = p;
int i = q;
we should be able to provide a fix-it hint, something like 'did you mean
to dereference q?'.  With the current PEDWARN_FOR_ASSIGNMENT macro it
would be awkward to implement that.


There are still more warnings to improve but I think better to do this
incrementally rather than a single humongous patch.


That makes sense.  I was going to mention that it would be nice
to also improve:

  warning: comparison of distinct pointer types lacks a cast

If you take the conversion suggestion I think this warning would
need to be phrased in terms of "conversion between T* and U*"
rather than "conversion from T* to U*".  (A similar change could
be made to the error message printed when incompatible pointers
are subtracted from one another.)


Sure.

Marek


Marek

Re: C PATCH to display types when printing a conversion warning (PR c/81233)

[Martin Sebor]

On 07/14/2017 07:47 AM, Marek Polacek wrote:

On Fri, Jul 14, 2017 at 02:52:36PM +0200, Marek Polacek wrote:

On Thu, Jul 13, 2017 at 11:42:15AM -0600, Martin Sebor wrote:

On 07/13/2017 08:18 AM, Marek Polacek wrote:

This patch improves diagnostic in the C FE by printing the types when reporting
a problem with a conversion.  E.g., instead of

   warning: assignment from incompatible pointer type

you'll now get

  warning: assignment to 'int *' from incompatible pointer type 'char *'

or instead of

  warning: initialization makes integer from pointer without a cast

this

   warning: initialization of 'int *' from 'int' makes pointer from integer 
without a cast

I've been wanting this for a long time and here it is.  Two snags: I had to
make pedwarn_init to take '...' for which I had to introduce
emit_diagnostic_valist; you can't pass varargs from one vararg function to
another vararg function (and a macro with __VA_ARGS__ didn't work here).  Also,
PEDWARN_FOR_ASSIGNMENT didn't work with the addition of printing TYPE and
RHSTYPE so I just decided to unroll the macro instead of making it even more
ugly.  This patch is long but it's mainly because of the testsuite fallout.

If you have better ideas about the wording, let me know.


It looks pretty good as is.  My only wording suggestion is to
consider simply mentioning conversion in the text of the warnings:

  warning: conversion to T* from an incompatible type U*

I'm not sure that being explicit about the context where the bad
conversion takes place (initialization vs assignment vs returning
a value) is terribly helpful.  That would not only simplify the
code and make all the messages consistent, but it would also make
it possible to get rid of the note when passing arguments.


Yeah, I agree, actually.  We print the expressions in question (although,
we could probably do even better), and I don't see why it would be
necessary to mention whether it's an initialization or an assignment.
I think I'll just drop this patch and do something along the lines you
suggest.

David, do you agree with this?  (Joseph's on PTO but I'd of course like
to hear his opinion, too.)


I think I changed my mind.  Because clang says e.g.
warning: returning 'unsigned int *' from a function with result type 'int *'
  converts between pointers to integer types with different sign
so in the end it might be best to just go with my current patch; it should
only improve things anyway.  And then add the fix-it hint.


Yes, Clang does do that.  G++, OTOH, prints an error with the same
text in all these cases.  It's not tremendously important which of
the two forms is used.  What I do think would be nice is if the text
of the same diagnostics, whether warnings or errors, could be more
consistent between the front ends.  A good way to do that in general,
if all of the checking code cannot be shared, is to provide a shared
diagnose_this_or_that() function for each diagnostic.  The function
would be parameterized on the kind of diagnostic (i.e., error or
warning), but would hardcode the shared text.  Each FE would call
it with an argument telling it whether to issue it as an error or
warning.

Martin


One more thing: for

int *q = p;
int i = q;
we should be able to provide a fix-it hint, something like 'did you mean
to dereference q?'.  With the current PEDWARN_FOR_ASSIGNMENT macro it
would be awkward to implement that.


There are still more warnings to improve but I think better to do this
incrementally rather than a single humongous patch.


That makes sense.  I was going to mention that it would be nice
to also improve:

  warning: comparison of distinct pointer types lacks a cast

If you take the conversion suggestion I think this warning would
need to be phrased in terms of "conversion between T* and U*"
rather than "conversion from T* to U*".  (A similar change could
be made to the error message printed when incompatible pointers
are subtracted from one another.)


Sure.

Marek


Marek

Re: C PATCH to display types when printing a conversion warning (PR c/81233)

[Marek Polacek]

On Fri, Jul 14, 2017 at 02:52:36PM +0200, Marek Polacek wrote:
> On Thu, Jul 13, 2017 at 11:42:15AM -0600, Martin Sebor wrote:
> > On 07/13/2017 08:18 AM, Marek Polacek wrote:
> > > This patch improves diagnostic in the C FE by printing the types when 
> > > reporting
> > > a problem with a conversion.  E.g., instead of
> > > 
> > >warning: assignment from incompatible pointer type
> > > 
> > > you'll now get
> > > 
> > >   warning: assignment to 'int *' from incompatible pointer type 'char *'
> > > 
> > > or instead of
> > > 
> > >   warning: initialization makes integer from pointer without a cast
> > > 
> > > this
> > > 
> > >warning: initialization of 'int *' from 'int' makes pointer from 
> > > integer without a cast
> > > 
> > > I've been wanting this for a long time and here it is.  Two snags: I had 
> > > to
> > > make pedwarn_init to take '...' for which I had to introduce
> > > emit_diagnostic_valist; you can't pass varargs from one vararg function to
> > > another vararg function (and a macro with __VA_ARGS__ didn't work here).  
> > > Also,
> > > PEDWARN_FOR_ASSIGNMENT didn't work with the addition of printing TYPE and
> > > RHSTYPE so I just decided to unroll the macro instead of making it even 
> > > more
> > > ugly.  This patch is long but it's mainly because of the testsuite 
> > > fallout.
> > > 
> > > If you have better ideas about the wording, let me know.
> > 
> > It looks pretty good as is.  My only wording suggestion is to
> > consider simply mentioning conversion in the text of the warnings:
> > 
> >   warning: conversion to T* from an incompatible type U*
> > 
> > I'm not sure that being explicit about the context where the bad
> > conversion takes place (initialization vs assignment vs returning
> > a value) is terribly helpful.  That would not only simplify the
> > code and make all the messages consistent, but it would also make
> > it possible to get rid of the note when passing arguments.
>  
> Yeah, I agree, actually.  We print the expressions in question (although,
> we could probably do even better), and I don't see why it would be
> necessary to mention whether it's an initialization or an assignment.
> I think I'll just drop this patch and do something along the lines you
> suggest.
> 
> David, do you agree with this?  (Joseph's on PTO but I'd of course like
> to hear his opinion, too.)
 
I think I changed my mind.  Because clang says e.g.
warning: returning 'unsigned int *' from a function with result type 'int *'
  converts between pointers to integer types with different sign
so in the end it might be best to just go with my current patch; it should
only improve things anyway.  And then add the fix-it hint.

> One more thing: for 
> 
> int *q = p;
> int i = q;
> we should be able to provide a fix-it hint, something like 'did you mean
> to dereference q?'.  With the current PEDWARN_FOR_ASSIGNMENT macro it
> would be awkward to implement that.
> 
> > > There are still more warnings to improve but I think better to do this
> > > incrementally rather than a single humongous patch.
> > 
> > That makes sense.  I was going to mention that it would be nice
> > to also improve:
> > 
> >   warning: comparison of distinct pointer types lacks a cast
> > 
> > If you take the conversion suggestion I think this warning would
> > need to be phrased in terms of "conversion between T* and U*"
> > rather than "conversion from T* to U*".  (A similar change could
> > be made to the error message printed when incompatible pointers
> > are subtracted from one another.)
> 
> Sure.
> 
>   Marek

Marek

Re: C PATCH to display types when printing a conversion warning (PR c/81233)

[Marek Polacek]

On Thu, Jul 13, 2017 at 11:42:15AM -0600, Martin Sebor wrote:
> On 07/13/2017 08:18 AM, Marek Polacek wrote:
> > This patch improves diagnostic in the C FE by printing the types when 
> > reporting
> > a problem with a conversion.  E.g., instead of
> > 
> >warning: assignment from incompatible pointer type
> > 
> > you'll now get
> > 
> >   warning: assignment to 'int *' from incompatible pointer type 'char *'
> > 
> > or instead of
> > 
> >   warning: initialization makes integer from pointer without a cast
> > 
> > this
> > 
> >warning: initialization of 'int *' from 'int' makes pointer from integer 
> > without a cast
> > 
> > I've been wanting this for a long time and here it is.  Two snags: I had to
> > make pedwarn_init to take '...' for which I had to introduce
> > emit_diagnostic_valist; you can't pass varargs from one vararg function to
> > another vararg function (and a macro with __VA_ARGS__ didn't work here).  
> > Also,
> > PEDWARN_FOR_ASSIGNMENT didn't work with the addition of printing TYPE and
> > RHSTYPE so I just decided to unroll the macro instead of making it even more
> > ugly.  This patch is long but it's mainly because of the testsuite fallout.
> > 
> > If you have better ideas about the wording, let me know.
> 
> It looks pretty good as is.  My only wording suggestion is to
> consider simply mentioning conversion in the text of the warnings:
> 
>   warning: conversion to T* from an incompatible type U*
> 
> I'm not sure that being explicit about the context where the bad
> conversion takes place (initialization vs assignment vs returning
> a value) is terribly helpful.  That would not only simplify the
> code and make all the messages consistent, but it would also make
> it possible to get rid of the note when passing arguments.
 
Yeah, I agree, actually.  We print the expressions in question (although,
we could probably do even better), and I don't see why it would be
necessary to mention whether it's an initialization or an assignment.
I think I'll just drop this patch and do something along the lines you
suggest.

David, do you agree with this?  (Joseph's on PTO but I'd of course like
to hear his opinion, too.)

One more thing: for 

int *q = p;
int i = q;
we should be able to provide a fix-it hint, something like 'did you mean
to dereference q?'.  With the current PEDWARN_FOR_ASSIGNMENT macro it
would be awkward to implement that.

> > There are still more warnings to improve but I think better to do this
> > incrementally rather than a single humongous patch.
> 
> That makes sense.  I was going to mention that it would be nice
> to also improve:
> 
>   warning: comparison of distinct pointer types lacks a cast
> 
> If you take the conversion suggestion I think this warning would
> need to be phrased in terms of "conversion between T* and U*"
> rather than "conversion from T* to U*".  (A similar change could
> be made to the error message printed when incompatible pointers
> are subtracted from one another.)

Sure.

Marek

Re: C PATCH to display types when printing a conversion warning (PR c/81233)

[Martin Sebor]

On 07/13/2017 08:18 AM, Marek Polacek wrote:

This patch improves diagnostic in the C FE by printing the types when reporting
a problem with a conversion.  E.g., instead of

   warning: assignment from incompatible pointer type

you'll now get

  warning: assignment to 'int *' from incompatible pointer type 'char *'

or instead of

  warning: initialization makes integer from pointer without a cast

this

   warning: initialization of 'int *' from 'int' makes pointer from integer 
without a cast

I've been wanting this for a long time and here it is.  Two snags: I had to
make pedwarn_init to take '...' for which I had to introduce
emit_diagnostic_valist; you can't pass varargs from one vararg function to
another vararg function (and a macro with __VA_ARGS__ didn't work here).  Also,
PEDWARN_FOR_ASSIGNMENT didn't work with the addition of printing TYPE and
RHSTYPE so I just decided to unroll the macro instead of making it even more
ugly.  This patch is long but it's mainly because of the testsuite fallout.

If you have better ideas about the wording, let me know.


It looks pretty good as is.  My only wording suggestion is to
consider simply mentioning conversion in the text of the warnings:

  warning: conversion to T* from an incompatible type U*

I'm not sure that being explicit about the context where the bad
conversion takes place (initialization vs assignment vs returning
a value) is terribly helpful.  That would not only simplify the
code and make all the messages consistent, but it would also make
it possible to get rid of the note when passing arguments.



There are still more warnings to improve but I think better to do this
incrementally rather than a single humongous patch.


That makes sense.  I was going to mention that it would be nice
to also improve:

  warning: comparison of distinct pointer types lacks a cast

If you take the conversion suggestion I think this warning would
need to be phrased in terms of "conversion between T* and U*"
rather than "conversion from T* to U*".  (A similar change could
be made to the error message printed when incompatible pointers
are subtracted from one another.)

Martin

C PATCH to display types when printing a conversion warning (PR c/81233)

[Marek Polacek]

This patch improves diagnostic in the C FE by printing the types when reporting
a problem with a conversion.  E.g., instead of 

   warning: assignment from incompatible pointer type

you'll now get

  warning: assignment to 'int *' from incompatible pointer type 'char *'

or instead of

  warning: initialization makes integer from pointer without a cast

this

   warning: initialization of 'int *' from 'int' makes pointer from integer 
without a cast

I've been wanting this for a long time and here it is.  Two snags: I had to
make pedwarn_init to take '...' for which I had to introduce
emit_diagnostic_valist; you can't pass varargs from one vararg function to
another vararg function (and a macro with __VA_ARGS__ didn't work here).  Also,
PEDWARN_FOR_ASSIGNMENT didn't work with the addition of printing TYPE and
RHSTYPE so I just decided to unroll the macro instead of making it even more
ugly.  This patch is long but it's mainly because of the testsuite fallout.

If you have better ideas about the wording, let me know.

There are still more warnings to improve but I think better to do this
incrementally rather than a single humongous patch.

Bootstrapped/regtested on x86_64-linux and powerpc64le-unknown-linux-gnu,
ok for trunk?

2017-07-13  Marek Polacek  

PR c/81233
* c-typeck.c (pedwarn_init): Make the function take a variable list.
Call emit_diagnostic_valist instead of pedwarn.
(convert_for_assignment): Unroll the PEDWARN_FOR_ASSIGNMENT macro.
Print the relevant types in diagnostics.

* diagnostic-core.h (emit_diagnostic_valist): Add declaration.
* diagnostic.c (emit_diagnostic): Add a comment.
(emit_diagnostic_valist): New function.

* gcc.dg/diagnostic-types-1.c: New test.
* gcc.dg/assign-warn-1.c: Update warning messages.
* gcc.dg/assign-warn-2.c: Likewise.
* gcc.dg/c90-const-expr-5.c: Likewise.
* gcc.dg/c99-const-expr-5.c: Likewise.
* gcc.dg/conv-2.c: Likewise.
* gcc.dg/init-bad-7.c: Likewise.
* gcc.dg/overflow-warn-1.c: Likewise.
* gcc.dg/overflow-warn-2.c: Likewise.
* gcc.dg/overflow-warn-3.c: Likewise.
* gcc.dg/overflow-warn-4.c: Likewise.
* gcc.dg/pointer-array-atomic.c: Likewise.
* gcc.dg/pr26865.c: Likewise.
* gcc.dg/pr61162-2.c: Likewise.
* gcc.dg/pr61162.c: Likewise.
* gcc.dg/pr67730-2.c: Likewise.
* gcc.dg/pr69156.c: Likewise.
* gcc.dg/pr70174.c: Likewise.
* objc.dg/proto-lossage-4.m: Likewise.

diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index 4d067e96dd3..742c047f7d1 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -6055,20 +6055,19 @@ error_init (location_t loc, const char *gmsgid)
it is unconditionally given.  GMSGID identifies the message.  The
component name is taken from the spelling stack.  */
 
-static void
-pedwarn_init (location_t loc, int opt, const char *gmsgid)
+static void ATTRIBUTE_GCC_DIAG (3,0)
+pedwarn_init (location_t loc, int opt, const char *gmsgid, ...)
 {
-  char *ofwhat;
-  bool warned;
-
   /* Use the location where a macro was expanded rather than where
  it was defined to make sure macros defined in system headers
  but used incorrectly elsewhere are diagnosed.  */
   source_location exploc = expansion_point_location_if_in_system_header (loc);
 
-  /* The gmsgid may be a format string with %< and %>. */
-  warned = pedwarn (exploc, opt, gmsgid);
-  ofwhat = print_spelling ((char *) alloca (spelling_length () + 1));
+  va_list ap;
+  va_start (ap, gmsgid);
+  bool warned = emit_diagnostic_valist (DK_PEDWARN, exploc, opt, gmsgid, );
+  va_end (ap);
+  char *ofwhat = print_spelling ((char *) alloca (spelling_length () + 1));
   if (*ofwhat && warned)
 inform (exploc, "(near initialization for %qs)", ofwhat);
 }
@@ -6301,17 +6300,33 @@ convert_for_assignment (location_t location, location_t 
expr_loc, tree type,
   if (checktype != error_mark_node
  && TREE_CODE (type) == ENUMERAL_TYPE
  && TYPE_MAIN_VARIANT (checktype) != TYPE_MAIN_VARIANT (type))
-   {
- PEDWARN_FOR_ASSIGNMENT (location, expr_loc, OPT_Wc___compat,
- G_("enum conversion when passing argument "
-"%d of %qE is invalid in C++"),
- G_("enum conversion in assignment is "
-"invalid in C++"),
- G_("enum conversion in initialization is "
-"invalid in C++"),
- G_("enum conversion in return is "
-"invalid in C++"));
-   }
+   switch (errtype)
+ {
+ case ic_argpass:
+   if (pedwarn (expr_loc, OPT_Wc___compat, "enum conversion when "
+"passing argument %d of %qE is invalid in C++",
+