FW: Review of Peter Bacon Darwin's Fixnum patch

John_LSSDLR · December 17, 2007, 6:51pm

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

John_LSSDLR · December 17, 2007, 9:05pm

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

John_LSSDLR · December 17, 2007, 11:55pm

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

John_LSSDLR · December 18, 2007, 8:05pm

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?

John_LSSDLR · December 19, 2007, 12:40am

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

John_LSSDLR · December 18, 2007, 9:50pm

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