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
