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

Peter Bacon Darwin bacondarwin at googlemail.com
Mon Dec 17 15:05:11 EST 2007

Thanks for doing the review of the contribution.  Hope you enjoyed the

I have added my own comments and answered your questions inline below:



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


[Pete] Yes, there  was a small issue with inspect that was causing some of
the Fixnum tests to fail.

I added ObjectOps.New because without it you couldn't instantiate a instance
of Object.  Object is not an abstract class. I believe that this is quite
common Ruby behavior, as you can then add singleton methods to the object
that is created.  This is exactly what I have done to create specific object
that respond in certain ways as fixtures in some of the tests.


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


[Pete] I suppose I was being a bit lazy:  As John notes Math.Pow takes
doubles so couldn't rely on that producing the same result; rather than code
up the whole algorithm again from scratch I just reused the BigInteger
method.  This can be fixed up if there is a performance issue.


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.


[Pete] I agree with having Protocols.IsEqual and Protocols.IsTrue.  Sorry I
unnecessarily added the NumericSites.Equal method.


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


[Pete] There are points in MRI where only to_int and the full conversion
protocol is not followed.  It may be that this is an oversight in the
implementation.  I expect that the behavior would be equivalent, so I will
revert to using Protocols.ConvertToInteger instead.


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


[Pete] Absolutely, along with CoerceAndCallCompare and
CoerceAndCallRelationOperator  Also Numeric.MakeCoercionError and
Numeric.MakeComparisonError could be moved to RubyExceptions.


[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). 

[Pete] Protocols.cs and of course RubySites.cs are indeed in IronRuby.dll,
so that is why I have been using Numeric.cs and NumericSites.cs to catch
these general cases.

The general pattern I've noticed is that sites are not normally needed from
library methods (except block sites).

[Pete] Not sure what you mean here.  All the sites I created in NumericSites
are being used in the Numeric classes; look at all the calls to


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. 


[Pete] I agree, but I couldn't see how to easily pull it out of each method
without making the methods less clear and not jumping through unnecessary
coding hoops.


There's some code duplication in Numeric.Step which could be simplified

[Pete] The trouble with the duplication is that it is Type specific.
Perhaps this could be achieved with a generic method?


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

[Pete] You could well be right here.  I can look into it if you want.


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.)

[Pete] This sounds like a good idea, unless one can monkey-patch the
behavior in Ruby by overriding the singleton_method_added method.  I suspect
this is not what was intended and blocking it at the immediate value level
is the correct way to go.





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

More information about the Ironruby-core mailing list