Ironruby core - Nov 2007 - FW: Can someone do a review of Pete's patch so that we can submit?

Review from Tomas

From: Tomas Matousek
Sent: Thursday, November 29, 2007 12:32 PM
To: John Lam (DLR); IronRuby Team
Subject: RE: Can someone do a review of Pete''s patch so that we can
submit?

Few issues:


-          Fixnum.Abs is incorrect: abs(Int32.MinValue) will blow up, it should
overlflow to BigInteger.

-          Why FloatOps.ToInteger doesn''t just do try { return
(int)self; } catch (...) {...} ? Flooring is actually incorrect: (-0.4).to_i
gives 0 in Ruby while -1 in IronRuby. A test for this is apparently missing.

-          Why FloatOps.Divmod doesn''t use Math.IEEEReminder? There
might be some precision peculiarity involved.

Tomas

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
http://rubyforge.org/pipermail/ironruby-core/attachments/20071129/4fb0de73/attachment.html

Hi Tomas,

I was only focussing on Bignum methods.  The other ones in  Fixnum and Float
were hacked together to support getting the specs to pass.  I was going to
look at Fixnum and Float next.  Unfortunately, due to the coercion
functionality in Ruby, it is not possible to completely isolate any of the
numeric types from each other to unit test.

Pete

 

From: ironruby-core-bounces at rubyforge.org
[mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of John Lam (DLR)
Sent: Thursday,29 November 29, 2007 22:13
To: ironruby-core at rubyforge.org
Subject: [Ironruby-core] FW: Can someone do a review of Pete''s patch so
that
we can submit?

 

Review from Tomas

 

From: Tomas Matousek 
Sent: Thursday, November 29, 2007 12:32 PM
To: John Lam (DLR); IronRuby Team
Subject: RE: Can someone do a review of Pete''s patch so that we can
submit?

 

Few issues:

 

-          Fixnum.Abs is incorrect: abs(Int32.MinValue) will blow up, it
should overlflow to BigInteger.

-          Why FloatOps.ToInteger doesn''t just do try { return
(int)self; }
catch (.) {.} ? Flooring is actually incorrect: (-0.4).to_i gives 0 in Ruby
while -1 in IronRuby. A test for this is apparently missing.

-          Why FloatOps.Divmod doesn''t use Math.IEEEReminder? There
might be
some precision peculiarity involved.

 

Tomas

 

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
http://rubyforge.org/pipermail/ironruby-core/attachments/20071130/f653459e/attachment.html

Sorry if my last mail on this subject sounded a bit defensive.  Thanks for
reviewing the patch I sent in.  I am really pleased that there were no
serious issues with the code I wrote for the Bignum class and this gives me
confidence to get on with the rest of the numeric classes.

Cheers,

Pete

 

From: ironruby-core-bounces at rubyforge.org
[mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of John Lam (DLR)
Sent: Thursday,29 November 29, 2007 22:13
To: ironruby-core at rubyforge.org
Subject: [Ironruby-core] FW: Can someone do a review of Pete''s patch so
that
we can submit?

 

Review from Tomas

 

From: Tomas Matousek 
Sent: Thursday, November 29, 2007 12:32 PM
To: John Lam (DLR); IronRuby Team
Subject: RE: Can someone do a review of Pete''s patch so that we can
submit?

 

Few issues:

 

-          Fixnum.Abs is incorrect: abs(Int32.MinValue) will blow up, it
should overlflow to BigInteger.

-          Why FloatOps.ToInteger doesn''t just do try { return
(int)self; }
catch (.) {.} ? Flooring is actually incorrect: (-0.4).to_i gives 0 in Ruby
while -1 in IronRuby. A test for this is apparently missing.

-          Why FloatOps.Divmod doesn''t use Math.IEEEReminder? There
might be
some precision peculiarity involved.

 

Tomas

 

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
http://rubyforge.org/pipermail/ironruby-core/attachments/20071201/29021813/attachment.html