Forum: IronRuby Review: fixes for Array#rindex and Array#reverse_each

Hi,
I just pushed two fixes on my repository, the first one addresses a
bug in Array#rindex (there was a bug in my last commit) and the second
one makes Array#reverse_each compliant with the rubyspecs.

http://github.com/nrk/ironruby/commit/d2b18f5d01a49cb62a2ea0c205e1cf1233ac94e0

>From the commit message:

* Fixed a bug in IListOps.ReverseIndex (core/array/rindex specs were
passing, this bug was triggered under certain conditions different from
the ones defined in the specs)

* Fixed ArrayOps.ReverseEach to make it not fail when elemens in the 
array
are removed from inside a block.

See also the attached diff.

Thanks,
Daniele

Can you add specs for rindex that expose the bug you fixed? Also, is 
there any shared place that you could put the following code:
         if (self.Count < originalSize) {
             i = originalSize - i - 1 + self.Count;
             originalSize = self.Count;
         }

It would be nice to get rid of the duplicated logic, but I can't think 
of where it should go.

Other than that, looks good.
JD

Ha! I got caught :-) The duplicated logic is there precisely because
I, too, couldn't think of a place to share that small piece of code in
a good way, so I just coded it like that for now but I'll ponder about
that for the next commit. OK for the specs.

Thanks,
Daniele

On Thu, Apr 23, 2009 at 22:08, Jim Deville <jdeville@microsoft.com> 
wrote:
>
>> in Array#rindex (there was a bug in my last commit) and the second one
>>
>> http://www.clorophilla.net/blog/
>> http://twitter.com/JoL1hAHN
> _______________________________________________
> Ironruby-core mailing list
> Ironruby-core@rubyforge.org
> http://rubyforge.org/mailman/listinfo/ironruby-core
>



--
Daniele Alessandri
http://www.clorophilla.net/blog/
http://twitter.com/JoL1hAHN

Hi Jim,

I'm a bit late but daytime work got in the way and then I was out of
the country for two weeks :-)
I got rid of the duplicated logic in IListOps.ReverseIndex and
ArrayOps.ReverseEach by implementing a new internal method
(IListOps.ReverseEnumerateIndexes):

http://github.com/nrk/ironruby/commit/76db817d1766b788f995afb02e12c5a72955c77f

As for the specs, I just slightly modified an existing one in a way
that does not affect the test but helped me to discover the bug
(actually this change uses a different condition from the one I used
one month ago as it exposed another bug which got fixed in the above
mentioned commit):

http://github.com/nrk/ironruby/commit/e5497cf87fc479b2bf2ca0b812d979354a86f44c

See also the attached diff.

Thanks,
Daniele

Hi,

It seems like this patch has gone unnoticed as it didn't get any
review yet so here I am, requesting for one :-)

Thanks,
Daniele

On Sun, May 31, 2009 at 13:22, Daniele Alessandri<suppakilla@gmail.com> 
wrote:
> As for the specs, I just slightly modified an existing one in a way
> Daniele
>>
>>>
>>> * Fixed a bug in IListOps.ReverseIndex (core/array/rindex specs were
>>>
>
>
> --
> Daniele Alessandri
> http://www.clorophilla.net/blog/
> http://twitter.com/JoL1hAHN
>



--
Daniele Alessandri
http://www.clorophilla.net/blog/
http://twitter.com/JoL1hAHN

Looks good. Nice job combining the logic. I think I went over this 
patch, but never sent a review mail, sorry about that!

I'll pull it in with the next pull.

JD

…there is no try

Thanks Jim!

BTW currently my repository is about one month behind. I must merge it 
with
the main repository but I can't do that now, I hope to make it by 
tomorrow
in the late afternoon.

Should be fine. I don’t plan on doing a pull until Friday.

From: ironruby-core-bounces@rubyforge.org 
[mailto:ironruby-core-bounces@rubyforge.org] On Behalf Of Daniele 
Alessandri
Sent: Wednesday, June 24, 2009 4:23 PM
To: ironruby-core@rubyforge.org
Subject: Re: [Ironruby-core] Review: fixes for Array#rindex and 
Array#reverse_each

Thanks Jim!

BTW currently my repository is about one month behind. I must merge it 
with the main repository but I can't do that now, I hope to make it by 
tomorrow in the late afternoon.


On Wed, Jun 24, 2009 at 23:42, Jim Deville 
<jdeville@microsoft.com<mailto:jdeville@microsoft.com>> wrote:
Looks good. Nice job combining the logic. I think I went over this 
patch, but never sent a review mail, sorry about that!

I'll pull it in with the next pull.

JD

…there is no try


> It seems like this patch has gone unnoticed as it didn't get any review yet so
> > the country for two weeks :-) I got rid of the duplicated logic in
> > that does not affect the test but helped me to discover the bug
> >
> >>         }
> >>> Sent: Thursday, April 23, 2009 12:59 PM
> http://github.com/nrk/ironruby/commit/d2b18f5d01a49cb62a2ea0c205e1cf
> >>> array are removed from inside a block.
> >> _______________________________________________
> > http://twitter.com/JoL1hAHN
> Ironruby-core@rubyforge.org<mailto:Ironruby-core@rubyforge.org>
> http://rubyforge.org/mailman/listinfo/ironruby-core
_______________________________________________
Ironruby-core mailing list
Ironruby-core@rubyforge.org<mailto:Ironruby-core@rubyforge.org>
http://rubyforge.org/mailman/listinfo/ironruby-core



--
Daniele Alessandri
http://www.clorophilla.net/blog/
http://twitter.com/JoL1hAHN

Just merged and pushed to my remote repository.

On Thu, Jun 25, 2009 at 01:29, Jim Deville<jdeville@microsoft.com> 
wrote:
> Array#reverse_each
>
>
>> Array#reverse_each
>>
>> >
>> >
>> > On Thu, Apr 23, 2009 at 22:08, Jim Deville <jdeville@microsoft.com>
>> >> of
>> >>> Subject: [Ironruby-core] Review: fixes for Array#rindex and
>> >>> 3ac94e0
>> >>> See also the attached diff.
>> >> Ironruby-core@rubyforge.org
>>
> _______________________________________________
> _______________________________________________
> Ironruby-core mailing list
> Ironruby-core@rubyforge.org
> http://rubyforge.org/mailman/listinfo/ironruby-core
>
>



--
Daniele Alessandri
http://www.clorophilla.net/blog/
http://twitter.com/JoL1hAHN