Review: fixes for Array#rindex and Array#reverse_each


#1

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.

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


#2

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


#3

Ha! I got caught :slight_smile: 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 D. removed_email_address@domain.invalid
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
removed_email_address@domain.invalid
http://rubyforge.org/mailman/listinfo/ironruby-core


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


#4

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


#5

Hi,

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

Thanks,
Daniele

On Sun, May 31, 2009 at 13:22, Daniele A.removed_email_address@domain.invalid
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 A.
http://www.clorophilla.net/blog/
http://twitter.com/JoL1hAHN


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


#6

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.


#7

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


#8

Just merged and pushed to my remote repository.

On Thu, Jun 25, 2009 at 01:29, Jim D.removed_email_address@domain.invalid
wrote:

Array#reverse_each

Array#reverse_each

On Thu, Apr 23, 2009 at 22:08, Jim D. removed_email_address@domain.invalid

of

Subject: [Ironruby-core] Review: fixes for Array#rindex and
3ac94e0
See also the attached diff.
removed_email_address@domain.invalid



Ironruby-core mailing list
removed_email_address@domain.invalid
http://rubyforge.org/mailman/listinfo/ironruby-core


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


#9

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

From: removed_email_address@domain.invalid
[mailto:removed_email_address@domain.invalid] On Behalf Of Daniele
Alessandri
Sent: Wednesday, June 24, 2009 4:23 PM
To: removed_email_address@domain.invalid
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 D.
<removed_email_address@domain.invalidmailto:removed_email_address@domain.invalid> 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 :slight_smile: 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
removed_email_address@domain.invalidmailto:removed_email_address@domain.invalid
http://rubyforge.org/mailman/listinfo/ironruby-core


Ironruby-core mailing list
removed_email_address@domain.invalidmailto:removed_email_address@domain.invalid
http://rubyforge.org/mailman/listinfo/ironruby-core


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