Comments

Attached is a patch that addresses most of Dodji Seketeli's comments.
Explanations for the rest are inline.
On 10/14/2013 03:04 AM, Dodji Seketeli wrote:
> Thank you Joshua for following up on this. Please find below some> comments of mine that mostly belong to the nitpicking department.
You are welcome, and thank you for looking at it and commenting on it.
> Joshua J Cogliati <jrincayc@yahoo.com> writes:> > [...]> >> A split out patch to make the floating conversions explicit is attached>> at the PR 53001 page, but is not included in this patch.> > It'd be nice to sent that split out patch here too, as it's a> pre-requisite for this to pass bootstrap w/o warning. Otherwise I am> afraid that patch isn't likely to get reviewed.
It is not a prerequisite of the attached patch
warn_float_patch_simple_trunk.diff. It becomes a pre-requisite if
-Wfloat-conversion is enabled with -Wextra. I did attached
fixup_float_conversions.diff that makes the changes needed in gcc.
> [...]> >> == Testcases ==>>>> This patch has passes the existing -Wconversion testcases. It modifies>> Wconversion-real.c, Wconversion-real-integer.c and pr35635.c to be more>> specific> > If the patch passes existing tests, I'd be inclined to leave them> tests alone and add new ones that are specific to what this patch> changes. You can even start from a copy of the tests you've modified> above and trim them down so that only exercise the new code paths> you've added; and of course add more cases top of these, if you can.> But I won't fight hard for this if the other maintainers think it's OK> this way. :-)
Hm. For gcc/testsuite/c-c++-common/Wconversion-real.c all the tests in
it are float-conversion by the patch's definition. For
gcc/testsuite/gcc.dg/Wconversion-real-integer.c it already had a mixture
of conversion and overflow, so it is now a mixture conversion,
float-conversion and overflow. Maybe gcc/testsuite/gcc.dg/pr35635.c
should stay the same. I don't think new tests are needed since the
existing ones can just be switched; otherwise the testcases would be
redundant. I certainly can change the patch if needed.
> [...]> >> == Changelog ==> > [...]> >> * gcc/c-family/c-common.c Switching unsafe_conversion_p to>> return an enumeration with more detail, and conversion_warning>> to use this information.> > The correct format for this ChangeLog entry would be:> > <tab>* gcc/c-family/c-common.c (unsafe_conversion_p): <insert-your-comment-here>.> > Please modify the rest of the ChangeLog entries to comply with this.> Sorry if this seems nit-picky but I guess we need to keep all the> ChangeLog entries coherent.
Here we go again:
Splitting out a -Wfloat-conversion from -Wconversion for
conversions that lower floating point number precision
or conversion from floating point numbers to integers
* gcc/c-family/c-common.c Switching unsafe_conversion_p to
return an enumeration with more detail, and conversion_warning
to use this information.
* gcc/c-family/c-common.h Adding conversion_safety enumeration
and switching return type of unsafe_conversion_p
* gcc/c-family/c.opt Adding new warning float-conversion and
enabling it -Wconversion
* gcc/doc/invoke.texi Adding documentation about
-Wfloat-conversion
* gcc/testsuite/c-c++-common/Wconversion-real.c Switching tests
to use float-conversion
* gcc/testsuite/gcc.dg/Wconversion-real-integer.c Switching
tests to use float-conversion
* gcc/testsuite/gcc.dg/pr35635.c Switching tests to use
float-conversion
> > [...]> >> +++ gcc/c-family/c-common.c (working copy)>> @@ -2517,10 +2517,10 @@ shorten_binary_op (tree result_type, tre>> Function allows conversions between types of different signedness and>> does not return true in that case. Function can produce signedness>> warnings if PRODUCE_WARNS is true. */>> -bool>> +enum conversion_safety>> unsafe_conversion_p (tree type, tree expr, bool produce_warns)>> {> > As you are modifying the return type of this function, I think you> should update the comment of the function too, especially the part> that talks about the return values.
Changed.
<snip>
> > You don't need the curly brackets here, so please move them.>
Changed.
<snip>
> >> +++ gcc/c-family/c-common.h (working copy)> > [...]> > >> +/* These variables are possible types of unsafe conversions. > > I would say "These enumerators", rather than "These variables".
Changed.
<snip>
> > Other than these nits, the patch looks nice to me.> > Thank you for working on this.>