Thank you Joshua for following up on this. Please find below some
comments of mine that mostly belong to the nitpicking department.
Joshua J Cogliati <jrinc...@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.
[...]
> == 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. :-)
[...]
> == 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.
[...]
> +++ 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.
[...]
> @@ -2689,8 +2689,9 @@ conversion_warning (tree type, tree expr
> {
> tree expr_type = TREE_TYPE (expr);
> location_t loc = EXPR_LOC_OR_HERE (expr);
> + enum conversion_safety conversion_kind;
>
> - if (!warn_conversion && !warn_sign_conversion)
> + if (!warn_conversion && !warn_sign_conversion && !warn_float_conversion)
> return;
>
> switch (TREE_CODE (expr))
> @@ -2717,10 +2718,19 @@ conversion_warning (tree type, tree expr
>
> case REAL_CST:
> case INTEGER_CST:
> - if (unsafe_conversion_p (type, expr, true))
> - warning_at (loc, OPT_Wconversion,
> - "conversion to %qT alters %qT constant value",
> - type, expr_type);
> + conversion_kind = unsafe_conversion_p (type, expr, true);
> + if(conversion_kind == UNSAFE_REAL)
> + {
> + warning_at (loc, OPT_Wfloat_conversion,
> + "conversion to %qT alters %qT constant value",
> + type, expr_type);
> + }
You don't need the curly brackets here, so please move them.
> + else if(conversion_kind)
> + {
> + warning_at (loc, OPT_Wconversion,
> + "conversion to %qT alters %qT constant value",
> + type, expr_type);
> + }
Likewise.
[...]
> return;
>
> case COND_EXPR:
> @@ -2736,10 +2746,19 @@ conversion_warning (tree type, tree expr
> }
>
> default: /* 'expr' is not a constant. */
> - if (unsafe_conversion_p (type, expr, true))
> - warning_at (loc, OPT_Wconversion,
> - "conversion to %qT from %qT may alter its value",
> - type, expr_type);
> + conversion_kind = unsafe_conversion_p (type, expr, true);
> + if(conversion_kind == UNSAFE_REAL)
> + {
> + warning_at (loc, OPT_Wfloat_conversion,
> + "conversion to %qT from %qT may alter its value",
> + type, expr_type);
> + }
Likewise.
> + else if(conversion_kind)
> + {
> + warning_at (loc, OPT_Wconversion,
> + "conversion to %qT from %qT may alter its value",
> + type, expr_type);
> + }
Likewise.
[...]
> +++ 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".
> + SAFE_CONVERSION The conversion is safe
> + UNSAFE_OTHER Another type of conversion with problems
> + UNSAFE_SIGN Conversion between signed and unsigned integers
> + which are all warned about immediately, so this is unused
> + UNSAFE_REAL Conversions that reduce the precision of reals
> + including conversions from reals to integers
> + */
> +enum conversion_safety { SAFE_CONVERSION = 0, UNSAFE_OTHER, UNSAFE_SIGN,
> UNSAFE_REAL };
> +
[...]
Other than these nits, the patch looks nice to me.
Thank you for working on this.
--
Dodji
