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

John Messerly jomes at microsoft.com
Mon Dec 17 17:55:03 EST 2007


Peter Bacon Darwin:

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

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

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.

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

Yup.

- John



More information about the Ironruby-core mailing list