Re: wide-int branch now up for public comment and review

[Mike Stump <mikestump at comcast dot net>]

On Aug 23, 2013, at 8:02 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote:
>> /* Negate THIS.  */
>> inline wide_int_ro
>> wide_int_ro::operator - () const
>> {
>>  wide_int_ro r;
>>  r = wide_int_ro (0) - *this;
>>  return r;
>> }
>> 
>> /* Negate THIS.  */
>> inline wide_int_ro
>> wide_int_ro::neg () const
>> {
>>  wide_int_ro z = wide_int_ro::from_shwi (0, precision);
>> 
>>  gcc_checking_assert (precision);
>>  return z - *this;
>> }
> 
> Why do we need both of these, and why are they implemented slightly
> differently?

Thanks for the review.

neg can go away.  There was a time when operator names weren't used, and I added them to make client code more natural.  I've removed neg () and updated neg (overflow) to match the style of operator -().

Index: wide-int.cc
===================================================================
--- wide-int.cc	(revision 201894)
+++ wide-int.cc	(working copy)
@@ -1545,7 +1545,7 @@ wide_int_ro::abs () const
   gcc_checking_assert (precision);
 
   if (sign_mask ())
-    result = neg ();
+    result = -*this;
   else
     result = *this;
 
Index: wide-int.h
===================================================================
--- wide-int.h	(revision 201950)
+++ wide-int.h	(working copy)
@@ -1800,19 +1800,7 @@ class GTY(()) wide_int_ro {
 
   /* Negate this.  */
   inline wide_int_ro operator - () const {
-    wide_int_ro r;
-    r = wide_int_ro (0) - *this;
-    return r;
-  }
-
-  /* Negate THIS.  */
-  inline wide_int_ro
-  neg () const
-  {
-    wide_int_ro z = wide_int_ro::from_shwi (0, precision);
-
-    gcc_checking_assert (precision);
-    return z - *this;
+    return wide_int_ro (0) - *this;
   }
 
   /* Negate THIS.  OVERFLOW is set true if the value cannot be
@@ -1820,12 +1808,11 @@ class GTY(()) wide_int_ro {
   inline wide_int_ro
   neg (bool *overflow) const
   {
-    wide_int_ro z = wide_int_ro::from_shwi (0, precision);
-
     gcc_checking_assert (precision);
+
     *overflow = only_sign_bit_p ();
 
-    return z - *this;
+    return wide_int_ro (0) - *this;
   }
 
   wide_int_ro parity () const;

>> template <int bitsize>
>> inline bool
>> fixed_wide_int <bitsize>::multiple_of_p (const wide_int_ro &factor,
>> 					 signop sgn,
>> 					 fixed_wide_int *multiple) const
>> {
>>  return wide_int_ro::multiple_of_p (factor, sgn,
>> 				     reinterpret_cast <wide_int *> (multiple));
>> }
> 
> The patch has several instances of this kind of reinterpret_cast.
> It looks like an aliasing violation.

The cast is completely unneeded, so I removed it.  sdivmod_floor was of the same class, so I removed it as well.  These two uses of reinterpret_cast are a bit different from the rest of them.  I'll see about addressing the remaining ones in a followup.

diff --git a/gcc/wide-int.h b/gcc/wide-int.h
index 0a906a9..3fef0d5 100644
--- a/gcc/wide-int.h
+++ b/gcc/wide-int.h
@@ -3081,9 +3081,7 @@ public:
   bool multiple_of_p (const wide_int_ro &factor,
                      signop sgn,
                      fixed_wide_int *multiple) const {
-    return wide_int_ro::multiple_of_p (factor,
-                                      sgn,
-                                      reinterpret_cast <wide_int *> (multiple));
+    return wide_int_ro::multiple_of_p (factor, sgn, multiple);
   }
 
   /* Conversion to and from GMP integer representations.  */
@@ -3336,7 +3334,7 @@ public:
   }
   template <typename T>
   inline fixed_wide_int sdivmod_floor (const T &c, fixed_wide_int *mod) const {
-    return wide_int_ro::sdivmod_floor (c, reinterpret_cast <wide_int *> (mod));
+    return wide_int_ro::sdivmod_floor (c, mod);
   }
 
   /* Shifting rotating and extracting.  */