diff mbox

[PR,53001] Re: Patch to split out new warning flag for floating point conversion

Message ID 525E059C.2090505@yahoo.com
State New
Headers show

Commit Message

Joshua J Cogliati Oct. 16, 2013, 3:18 a.m. UTC 
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.
>
diff mbox

Patch

Index: gcc/c-family/c-cppbuiltin.c
===================================================================
--- gcc/c-family/c-cppbuiltin.c	(revision 203112)
+++ gcc/c-family/c-cppbuiltin.c	(working copy)
@@ -157,7 +157,7 @@  builtin_define_float_constants (const ch
 	p log10 b			if b is a power of 10
 	floor((p - 1) log10 b)		otherwise
   */
-  dig = (fmt->p - 1) * log10_b;
+  dig = (int)((fmt->p - 1) * log10_b);
   sprintf (name, "__%s_DIG__", name_prefix);
   builtin_define_with_int_value (name, dig);
 
@@ -173,7 +173,7 @@  builtin_define_float_constants (const ch
 
      Recall that emin is negative, so the integer truncation calculates
      the ceiling, not the floor, in this case.  */
-  min_10_exp = (fmt->emin - 1) * log10_b;
+  min_10_exp = (int)((fmt->emin - 1) * log10_b);
   sprintf (name, "__%s_MIN_10_EXP__", name_prefix);
   sprintf (buf, "(%d)", min_10_exp);
   builtin_define_with_value (name, buf, 0);
@@ -208,7 +208,7 @@  builtin_define_float_constants (const ch
      Hand-waving aside, crunching all of the sets of constants above by hand
      does not yield a case for which the first term is significant, which
      in the end is all that matters.  */
-  max_10_exp = fmt->emax * log10_b;
+  max_10_exp = (int)(fmt->emax * log10_b);
   sprintf (name, "__%s_MAX_10_EXP__", name_prefix);
   builtin_define_with_int_value (name, max_10_exp);
 
@@ -224,14 +224,14 @@  builtin_define_float_constants (const ch
   {
     double d_decimal_dig
       = 1 + (fmt->p < ldfmt->p ? ldfmt->p : fmt->p) * log10_b;
-    decimal_dig = d_decimal_dig;
+    decimal_dig = (int)d_decimal_dig;
     if (decimal_dig < d_decimal_dig)
       decimal_dig++;
   }
   /* Similar, for this type rather than long double.  */
   {
     double type_d_decimal_dig = 1 + fmt->p * log10_b;
-    type_decimal_dig = type_d_decimal_dig;
+    type_decimal_dig = (int)type_d_decimal_dig;
     if (type_decimal_dig < type_d_decimal_dig)
       type_decimal_dig++;
   }
Index: gcc/mcf.c
===================================================================
--- gcc/mcf.c	(revision 203112)
+++ gcc/mcf.c	(working copy)
@@ -348,8 +348,8 @@  mcf_sqrt (double x)
 
   gcc_assert (x >= 0);
 
-  convertor.floatPart = x;
-  convertor2.floatPart = x;
+  convertor.floatPart = (float)x;
+  convertor2.floatPart = (float)x;
   convertor.intPart = MAGIC_CONST1 + (convertor.intPart >> 1);
   convertor2.intPart = MAGIC_CONST2 - (convertor2.intPart >> 1);
 
Index: gcc/predict.c
===================================================================
--- gcc/predict.c	(revision 203112)
+++ gcc/predict.c	(working copy)
@@ -792,7 +792,7 @@  combine_predictions_for_insn (rtx insn,
 	  /* If one probability is 0% and one 100%, avoid division by zero.  */
 	  combined_probability = REG_BR_PROB_BASE / 2;
 	else
-	  combined_probability = (((double) combined_probability) * probability
+	  combined_probability = (int)(((double) combined_probability) * probability
 				  * REG_BR_PROB_BASE / d + 0.5);
       }
 
@@ -957,7 +957,7 @@  combine_predictions_for_bb (basic_block
 	    /* If one probability is 0% and one 100%, avoid division by zero.  */
 	    combined_probability = REG_BR_PROB_BASE / 2;
 	  else
-	    combined_probability = (((double) combined_probability)
+	    combined_probability = (int)(((double) combined_probability)
 				    * probability
 		    		    * REG_BR_PROB_BASE / d + 0.5);
 	}
Index: gcc/real.c
===================================================================
--- gcc/real.c	(revision 203112)
+++ gcc/real.c	(working copy)
@@ -1550,14 +1550,14 @@  real_to_decimal_for_mode (char *str, con
     }
 
   /* Bound the number of digits printed by the size of the representation.  */
-  max_digits = SIGNIFICAND_BITS * M_LOG10_2;
+  max_digits = (int)(SIGNIFICAND_BITS * M_LOG10_2);
   if (digits == 0 || digits > max_digits)
     digits = max_digits;
 
   /* Estimate the decimal exponent, and compute the length of the string it
      will print as.  Be conservative and add one to account for possible
      overflow or rounding error.  */
-  dec_exp = REAL_EXP (&r) * M_LOG10_2;
+  dec_exp = (int)(REAL_EXP (&r) * M_LOG10_2);
   for (max_digits = 1; dec_exp ; max_digits++)
     dec_exp /= 10;
 
@@ -2215,7 +2215,7 @@  decimal_integer_string (char *str, const
   sign = r.sign;
   r.sign = 0;
 
-  dec_exp = REAL_EXP (&r) * M_LOG10_2;
+  dec_exp = (int)(REAL_EXP (&r) * M_LOG10_2);
   digits = dec_exp + 1;
   gcc_assert ((digits + 2) < (int)buf_size);
 
@@ -2818,7 +2818,7 @@  significand_size (enum machine_mode mode
 	 than the number of bits required to hold the largest coefficient
 	 of this mode.  */
       double log2_10 = 3.3219281;
-      return fmt->p * log2_10;
+      return (int)(fmt->p * log2_10);
     }
   return fmt->p;
 }
Index: libcpp/symtab.c
===================================================================
--- libcpp/symtab.c	(revision 203112)
+++ libcpp/symtab.c	(working copy)
@@ -287,7 +287,8 @@  ht_dump_statistics (cpp_hash_table *tabl
 		     : (x) / (1024*1024))))
 #define LABEL(x) ((x) < 1024*10 ? ' ' : ((x) < 1024*1024*10 ? 'k' : 'M'))
 
-  total_bytes = longest = sum_of_squares = nids = 0;
+  total_bytes = longest = nids = 0;
+  sum_of_squares = 0.0;
   p = table->entries;
   limit = p + table->nslots;
   do