Forum: IronRuby Fwd: WorkItem Issue #6095 - "IronRuby 1.1.3 Can't count"

B20991026b45ee0d77e9bbb2b6776097?d=identicon&s=25 Nicholas Radford (Guest)
on 2011-05-04 23:38
(Received via mailing list)
Hey all,

 I thought I'd look at issue #6095 on codeplex
(http://ironruby.codeplex.com/workitem/6095) as my first contribution.

 Having a dig through the code lead me to ArrayOps which then lead
me IListOps in the IronRuby.Libraries project.

 I noticed IListOps.Length had the [RubyMethod("count")] attribute
method which only returned the arrays count. Have to some digging (and
unneeded code writing then reverting)
 I looked at the ruby doc and saw Array mixed in Enumerable, looked
around and found that IListOps was indeed marked to include Enumerable
and sure enough found the 3 count methods.

 One for no parameters, one with, and one with a block that did all
they were supposed to do.

 So the RubyMethod declaration for count in IListOps was
overriding the method declarations in Enumerable once the ruby class
was put together at runtime.

 I removed the RubyMethod declaration on IListOps compiled and ran
the IronRuby console, tried out all three versions of count and they
all worked.

 Now, I can see why the count declaration on IListOps was put
there, because accessing the internal count property on RubyArray is
much quicker than counting and iterating over the collection, however,
the ruby methods length and size for quick access to the number of
elements in the RubyArray.

 I see two ways of fixing the bug,
  1. removing the [RubyMethod("count")] declaration from
IListOps.Length which would make count inefficent on IList's.
  2. reimplementing the object and block variations of count in IListOps

In my humble opinion, 1 would be best, but since this is my first
change I'd thought I'd get some input.

Cheers,

--
Nik Radford

Mobile: 07884 254 866
Email: nikradford@googlemail.com



--
Nik Radford

Mobile: 07884 254 866
Email: nikradford@googlemail.com
99b1f0c67bec23747d007e27d000487b?d=identicon&s=25 Ryan Riley (Guest)
on 2011-05-05 01:14
(Received via mailing list)
On Wed, May 4, 2011 at 2:02 PM, Nicholas Radford
<nik@terminaldischarge.net>wrote:

> Hey all,
>
>    I thought I'd look at issue #6095 on codeplex
> (http://ironruby.codeplex.com/workitem/6095) as my first contribution.
>


>    I see two ways of fixing the bug,
>      1. removing the [RubyMethod("count")] declaration from
> IListOps.Length which would make count inefficent on IList's.
>      2. reimplementing the object and block variations of count in IListOps
>
> In my humble opinion, 1 would be best, but since this is my first
> change I'd thought I'd get some input.
>

I'm partial to option 2. As Ruby is already a slow language, we should
try
to optimize it wherever possible. In addition, 2 keeps to the trend in
other
.NET languages. For example, I believe LINQ takes advantage of existing
methods on Lists when it can.

Ryan
3a9ff49a9e689dcbfc8242f05180cc31?d=identicon&s=25 Orion Edwards (Guest)
on 2011-05-05 05:04
(Received via mailing list)
>>
>>
> I'm partial to option 2. As Ruby is already a slow language, we should try
> to optimize it wherever possible. In addition, 2 keeps to the trend in other
> .NET languages. For example, I believe LINQ takes advantage of existing
> methods on Lists when it can.
>

Likewise. Because arrays are used absolutely everywhere in ruby, making
a
common method like count slower will have a whole lot of hidden
performance
penalties... Anything we can do to make these core classes faster is a
win
for everyone
B20991026b45ee0d77e9bbb2b6776097?d=identicon&s=25 Nicholas Radford (Guest)
on 2011-05-05 10:39
(Received via mailing list)
>> I'm partial tooption 2. As Ruby is already a slow language, we should try
>> to optimize it wherever possible. In addition, 2 keeps to the trend in other
>> .NET languages. For example, I believe LINQ takes advantage of existing
>> methods on Lists when it can.
>
> Likewise. Because arrays are used absolutely everywhere in ruby, making a
> common method like count slower will have a whole lot of hidden performance
> penalties... Anything we can do to make these core classes faster is a win
> for everyone

Alright, I'll get onto doing option 2 when I get home this evening.

Thanks guys.
B20991026b45ee0d77e9bbb2b6776097?d=identicon&s=25 Nicholas Radford (Guest)
on 2011-05-07 10:09
(Received via mailing list)
On 5 May 2011 09:32, Nicholas Radford <nik@terminaldischarge.net> wrote:
> Alright, I'll get onto doing option 2 when I get home this evening.
>
> Thanks guys.
>

Quick question guys, when pushing this to my repo on github, should I
push to a topic branch or push to master?

--
Nik Radford

Mobile: 07884 254 866
Email: nikradford@googlemail.com
B20991026b45ee0d77e9bbb2b6776097?d=identicon&s=25 Nicholas Radford (Guest)
on 2011-05-07 10:39
(Received via mailing list)
>
> Quick question guys, when pushing this to my repo on github, should I
> push to a topic branch or push to master?
>

Well I pushed it to a topic branch, just in case you guys required
changes :)

As the contributing documentation says, would someone mind giving me a
code review?

Code can be found at

https://github.com/sekhat/main/tree/bugfix/issue-6095

Cheers.
Cb51033949ffccd982ae32c9f890f25a?d=identicon&s=25 Tomas Matousek (Guest)
on 2011-05-08 03:50
(Received via mailing list)
Yes, this is the right process - fix it in a branch and send a pull
request.

The change looks great! Merged into main and closed the bug.

Thanks,
Tomas
This topic is locked and can not be replied to.