# FW: Review of Peter Bacon Darwin's Fixnum patch

**URL:** <https://www.ruby-forum.com/t/fw-review-of-peter-bacon-darwins-fixnum-patch/123432>\
**Category:** IronRuby\
**Created:** [December 17, 2007, 6:51pm UTC](https://www.ruby-forum.com/t/fw-review-of-peter-bacon-darwins-fixnum-patch/123432 "2007-12-17T18:51:53Z")\
**Posts on this page:** 6\
**Page:** 1

<div class="post-metadata">

**Author:** ![John\_LSSDLR](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/5f8ce5/32.png) [@John\_LSSDLR](https://www.ruby-forum.com/u/John_LSSDLR)\
**Post date:** [December 17, 2007, 6:51pm UTC](https://www.ruby-forum.com/t/fw-review-of-peter-bacon-darwins-fixnum-patch/123432/1 "2007-12-17T18:51:53Z")

</div>

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 M.  
Sent: Thursday, December 13, 2007 8:45 PM  
To: John L. (DLR); IronRuby Team  
Subject: RE: Review of Peter Bacon D.'s Fixnum patch

[inline]

From: John L. (DLR)  
Sent: Thursday, December 13, 2007 7:28 PM  
To: IronRuby Team  
Subject: Review of Peter Bacon D.'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

---

<div class="post-metadata">

**Author:** ![John\_LSSDLR](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/5f8ce5/32.png) [@John\_LSSDLR](https://www.ruby-forum.com/u/John_LSSDLR)\
**Post date:** [December 17, 2007, 9:05pm UTC](https://www.ruby-forum.com/t/fw-review-of-peter-bacon-darwins-fixnum-patch/123432/2 "2007-12-17T21:05:45Z")

</div>

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

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

Pete

From: John L. (DLR)  
Sent: Thursday, December 13, 2007 7:28 PM  
To: IronRuby Team  
Subject: Review of Peter Bacon D.'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  
CoerceAndCall.

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

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

Thanks,

-John

---

<div class="post-metadata">

**Author:** ![John\_LSSDLR](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/5f8ce5/32.png) [@John\_LSSDLR](https://www.ruby-forum.com/u/John_LSSDLR)\
**Post date:** [December 17, 2007, 11:55pm UTC](https://www.ruby-forum.com/t/fw-review-of-peter-bacon-darwins-fixnum-patch/123432/3 "2007-12-17T23:55:54Z")

</div>

Peter Bacon D.:

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

Yup–just needs to be moved to Kernel.cs. John, are you fixing this?

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

I don’t understand. I can type Object.new at an IronRuby console and it  
just works:

> > > $x = Object.new  
> > > =\> #Object:0x000005a
> 
> > > def $x.foo; “called foo”; end  
> > > =\> nil
> 
> > > $x.foo  
> > > =\> “called foo”

(the method binder finds the CLR System.Object constructor & calls it)  
In what case doesn’t it work?

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

Agreed. I say we keep the code as is & then change it if it proves to be  
a performance bottleneck.

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

If so, could we create new protocols? Or are the conversions really only  
used in one place? If that’s the case, then it’s fine to call “to\_int”  
directly. But so far most of the conversion protocols seem to recur  
often (I’m guessing they’re C macros/helper functions in MRI)

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

Yeah, I think we should move Protocols to IronRuby.Libraries. Well, feel  
free to add code there in the meantime–it’s library functionality  
anyway.

> method without making the methods less clear and not jumping through  
> unnecessary coding hoops.

I just mean that it could go in Protocols.Compare itself. I’m guessing  
that most places will throw if the comparer returns nil. If not, we can  
make Protocols.TryCompare & Protocols.Compare–the latter will throw.

> > 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?

Yeah, if possible. But if there’s no obvious way to clean it up, don’t  
worry about it.

> > 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.
> 
> [Pete] You could well be right here. I can look into it if you want.

That would be great. The question is: if you break from the block, does  
the correct value get returned? My understanding was that you had to  
return the value the block returned, but maybe that isn’t required.

> value level is the correct way to go.  
> Yup.

- John

---

<div class="post-metadata">

**Author:** ![John\_LSSDLR](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/5f8ce5/32.png) [@John\_LSSDLR](https://www.ruby-forum.com/u/John_LSSDLR)\
**Post date:** [December 18, 2007, 8:05pm UTC](https://www.ruby-forum.com/t/fw-review-of-peter-bacon-darwins-fixnum-patch/123432/4 "2007-12-18T20:05:06Z")

</div>

Sorry about the Kernel stuff. I didn’t realise that most of the  
instance  
methods on Object are actually in Kernel (the documentation is a bit  
misleading there if you don’t read the top of the page carefully).  
Equally,  
I have no idea why I implemented Object.new. You are right it works  
fine.  
Perhaps I was having another problem and thought that would fix it and  
didn’t isolate the problem.

Regarding the to\_int site, I think that the cases where I used it are  
the  
only ones and everywhere else use the normal protocols. I think that in  
those few cases they are optimizations where the other conversions in  
the  
normal protocol have already been ruled out.

Sorry you are right about the YieldStep method. It should return the  
result  
of the block if it jumped. By the way, is there any difference between  
a  
block jumping with a break statement and jumping with a return  
statement?

---

<div class="post-metadata">

**Author:** ![John\_LSSDLR](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/5f8ce5/32.png) [@John\_LSSDLR](https://www.ruby-forum.com/u/John_LSSDLR)\
**Post date:** [December 19, 2007, 12:40am UTC](https://www.ruby-forum.com/t/fw-review-of-peter-bacon-darwins-fixnum-patch/123432/5 "2007-12-19T00:40:24Z")

</div>

I added Struct which was pushed out with the SVN update this morning.  
It’s passing all specs expect 4, the failures are:

```
* three because we're missing Kernel#methods, 

```

Kernel#private\_method–should pass once those methods are implemented  
\* one because of a known bug when you derive from Struct itself and  
call “new” on that class (deriving from classes created by Struct#new  
should work okay)

One more built-in type down… lots to go 🙂

FYI, I’m also working on “super” support.

- John

---

<div class="post-metadata">

**Author:** ![John\_LSSDLR](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/5f8ce5/32.png) [@John\_LSSDLR](https://www.ruby-forum.com/u/John_LSSDLR)\
**Post date:** [December 18, 2007, 9:50pm UTC](https://www.ruby-forum.com/t/fw-review-of-peter-bacon-darwins-fixnum-patch/123432/6 "2007-12-18T21:50:40Z")

</div>

Peter Bacon D.:

> Sorry you are right about the YieldStep method. It should return the  
> result of the block if it jumped. By the way, is there any difference  
> between a block jumping with a break statement and jumping with a  
> return statement?

Yeah, I made that change to YieldStep & checked in. Let me know if  
anything didn’t get merged correctly.

Return & break are different, but the control flow code  
(RuntimeFlowControl, BlockParam) tracks that stuff. The only thing  
library code needs to do is:

object result = \_MyBlockSite.Invoke(context, block, args …)  
if (block.BlockJumped(result)) {  
return result;  
}

Maybe in the future we can clean that up, but that’s the story for now.

- John