[Ironruby-core] FW: Review of Peter Bacon Darwin's Fixnum patch

[John Lam (DLR)]

FYI We did this internal review last week (we had a ski day interfere with work on Friday :))- am working on some minor changes to the shelveset before checking in later today.

Thanks!
-John


From: John Messerly
Sent: Thursday, December 13, 2007 8:45 PM
To: John Lam (DLR); IronRuby Team
Subject: RE: Review of Peter Bacon Darwin's Fixnum patch

[inline]

From: John Lam (DLR)
Sent: Thursday, December 13, 2007 7:28 PM
To: IronRuby Team
Subject: Review of Peter Bacon Darwin's Fixnum patch

tfpt review /shelveset:FixnumPatch;jflam

My initial look:

ObjectOps.cs: I don't understand why he's re-implementing new and inspect here - should revert this change.
[jomes] He's fixing our "inspect" method to call "to_s" if available. The change is correct:

irb(main):001:0> class Bob
irb(main):002:1> def to_s; "abc"; end
irb(main):003:1> end
=> nil
irb(main):004:0> Bob.new
=> abc
>>> ^Z

But it should be in ObjectOps. But I'm not sure about Object.new, probably ask why he added that

FixnumOps.cs (388): I don't understand why he's converting the Fixnum into a Bignum before doing the power operation.

[jomes] probably to get overflow right without a try-catch... also Math.Pow takes doubles

There appears to be a duplicate NumericSites.Equal and RubySites.Equal. After looking a bit more, it seems like we should introduce a new protocol:

public static bool IsEqual(CodeContext context, object lhs, object rhs) {
  return Protocol.IsTrue(RubySites.Equal(context, lhs, rhs));
}

We should move RubyOps.IsTrue to Protocols.

Some usage of RubySites.ToInt instead of Protocols.ToInt. Should we remove RubySites.ToInt altogether?

Numeric.CoerceAndCall is used in a lot of places - looks like a good candidate to make into a Protocol.

[jomes] agreed about all the protocol stuff-needs to be consistent & in Protocols.cs (actually, is this file in IronRuby.dll or Libraries? Maybe that was why he didn't put them there). The general pattern I've noticed is that sites are not normally needed from library methods (except block sites).

Also, Comparable uses this pattern everywhere now:

            object result = Protocols.Compare(context, self, other);
            if (result == null) {
                Numeric.MakeComparisonError(context, self, other);
            }

Which looks correct, but should be baked into the Protocol itself.

There's some code duplication in Numeric.Step which could be simplified
In Numeric.YieldStep, if the BlockJumped he's not returning the value that the block returned. But maybe that isn't necessary? Tomas would know for sure.

I think singleton_method_added shouldn't be on Numeric.... Probably we should throw instead when you try to define a singleton method (or any instance data) on an immediate value. (Ruby immediate values are true, false, nil, Fixnums, and Symbols. We have a check for this in RubyUtils.IsRubyValueType. But we don't think we enforce that you can't add singleton methods on them.)


Thanks,
-John

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://rubyforge.org/pipermail/ironruby-core/attachments/20071217/58c5dd1e/attachment-0001.html