Ironruby core - Dec 2007 - FW: Review of Peter Bacon Darwin's Fixnum patch

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

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

 

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

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

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?

Peter Bacon Darwin:
> 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

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