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
