Daniele Alessandri
2009-Apr-23 19:58 UTC
[Ironruby-core] 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 -- Daniele Alessandri http://www.clorophilla.net/blog/ http://twitter.com/JoL1hAHN -------------- next part -------------- A non-text attachment was scrubbed... Name: specs_core_array_090423.diff Type: application/octet-stream Size: 3641 bytes Desc: not available URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20090423/d01b4c3e/attachment.obj>
Jim Deville
2009-Apr-23 20:08 UTC
[Ironruby-core] Review: fixes for Array#rindex and Array#reverse_each
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
> -----Original Message-----
> From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-
> bounces at rubyforge.org] On Behalf Of Daniele Alessandri
> Sent: Thursday, April 23, 2009 12:59 PM
> To: ironruby-core at rubyforge.org
> Subject: [Ironruby-core] 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/d2b18f5d01a49cb62a2ea0c205e1cf123
> 3ac94e0
>
> >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
>
> --
> Daniele Alessandri
> http://www.clorophilla.net/blog/
> http://twitter.com/JoL1hAHN
Daniele Alessandri
2009-Apr-23 21:03 UTC
[Ironruby-core] Review: fixes for Array#rindex and Array#reverse_each
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 at microsoft.com> wrote:> 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 > >> -----Original Message----- >> From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core- >> bounces at rubyforge.org] On Behalf Of Daniele Alessandri >> Sent: Thursday, April 23, 2009 12:59 PM >> To: ironruby-core at rubyforge.org >> Subject: [Ironruby-core] 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/d2b18f5d01a49cb62a2ea0c205e1cf123 >> 3ac94e0 >> >> >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 >> >> -- >> Daniele Alessandri >> http://www.clorophilla.net/blog/ >> http://twitter.com/JoL1hAHN > _______________________________________________ > Ironruby-core mailing list > Ironruby-core at rubyforge.org > http://rubyforge.org/mailman/listinfo/ironruby-core >-- Daniele Alessandri http://www.clorophilla.net/blog/ http://twitter.com/JoL1hAHN
Daniele Alessandri
2009-May-31 11:22 UTC
[Ironruby-core] Review: fixes for Array#rindex and Array#reverse_each
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 On Thu, Apr 23, 2009 at 22:08, Jim Deville <jdeville at microsoft.com> wrote:> 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 > >> -----Original Message----- >> From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core- >> bounces at rubyforge.org] On Behalf Of Daniele Alessandri >> Sent: Thursday, April 23, 2009 12:59 PM >> To: ironruby-core at rubyforge.org >> Subject: [Ironruby-core] 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/d2b18f5d01a49cb62a2ea0c205e1cf123 >> 3ac94e0 >> >> >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 >> >> -- >> Daniele Alessandri >> http://www.clorophilla.net/blog/ >> http://twitter.com/JoL1hAHN > _______________________________________________ > Ironruby-core mailing list > Ironruby-core at rubyforge.org > http://rubyforge.org/mailman/listinfo/ironruby-core >-- Daniele Alessandri http://www.clorophilla.net/blog/ http://twitter.com/JoL1hAHN -------------- next part -------------- diff --git a/Merlin/External.LCA_RESTRICTED/Languages/IronRuby/mspec/rubyspec/core/array/rindex_spec.rb b/Merlin/External.LCA_RESTRICTED/Languages/IronRuby/mspec/rubyspec/core/array/rindex_spec.rb index 4971cf3..6e43c2b 100644 --- a/Merlin/External.LCA_RESTRICTED/Languages/IronRuby/mspec/rubyspec/core/array/rindex_spec.rb +++ b/Merlin/External.LCA_RESTRICTED/Languages/IronRuby/mspec/rubyspec/core/array/rindex_spec.rb @@ -17,31 +17,31 @@ describe "Array#rindex" do it "returns size-1 if last element == to object" do [2, 1, 3, 2, 5].rindex(5).should == 4 end it "returns 0 if only first element == to object" do [2, 1, 3, 1, 5].rindex(2).should == 0 end it "returns nil if no element == to object" do [1, 1, 3, 2, 1, 3].rindex(4).should == nil end it "does not fail when removing elements from block" do sentinel = mock(''sentinel'') - ary = [0, 0, 1, 1, 3, 2, 1, sentinel] + ary = [0, 0, 1, 1, 3, 2, 1, sentinel, 4] sentinel.instance_variable_set(:@ary, ary) def sentinel.==(o) @ary.slice!(1..-1); false; end ary.rindex(0).should == 0 end it "properly handles recursive arrays" do empty = ArraySpecs.empty_recursive_array empty.rindex(empty).should == 0 empty.rindex(1).should be_nil array = ArraySpecs.recursive_array array.rindex(1).should == 0 array.rindex([array]).should == 3 diff --git a/Merlin/Main/Languages/Ruby/Libraries.LCA_RESTRICTED/Builtins/ArrayOps.cs b/Merlin/Main/Languages/Ruby/Libraries.LCA_RESTRICTED/Builtins/ArrayOps.cs index f83c4c7..301007a 100644 --- a/Merlin/Main/Languages/Ruby/Libraries.LCA_RESTRICTED/Builtins/ArrayOps.cs +++ b/Merlin/Main/Languages/Ruby/Libraries.LCA_RESTRICTED/Builtins/ArrayOps.cs @@ -546,31 +546,27 @@ namespace IronRuby.Builtins { [RubyMethod("reverse!")] public static RubyArray/*!*/ InPlaceReverse(RubyContext/*!*/ context, RubyArray/*!*/ self) { RubyUtils.RequiresNotFrozen(context, self); self.Reverse(); return self; } [RubyMethod("reverse_each")] public static object ReverseEach(RubyContext/*!*/ context, BlockParam block, RubyArray/*!*/ self) { Assert.NotNull(context, self); if (self.Count > 0 && block == null) { throw RubyExceptions.NoBlockGiven(); } - for (int originalSize = self.Count, i = originalSize - 1; i >= 0; i--) { + foreach (int index in IListOps.ReverseEnumerateIndexes(self)) { object result; - if (block.Yield(self[i], out result)) { + if (block.Yield(self[index], out result)) { return result; } - if (self.Count < originalSize) { - i = originalSize - i - 1 + self.Count; - originalSize = self.Count; - } } return self; } #endregion } } diff --git a/Merlin/Main/Languages/Ruby/Libraries.LCA_RESTRICTED/Extensions/IListOps.cs b/Merlin/Main/Languages/Ruby/Libraries.LCA_RESTRICTED/Extensions/IListOps.cs index e1197ad..3224c55 100644 --- a/Merlin/Main/Languages/Ruby/Libraries.LCA_RESTRICTED/Extensions/IListOps.cs +++ b/Merlin/Main/Languages/Ruby/Libraries.LCA_RESTRICTED/Extensions/IListOps.cs @@ -138,30 +138,40 @@ namespace IronRuby.Builtins { if (array != null) { return array.CreateInstance(); } // interop - call a default ctor to get an instance: var allocate = allocateStorage.GetCallSite("allocate", 0); var cls = allocateStorage.Context.GetClassOf(list); var result = allocate.Target(allocate, cls) as IList; if (result != null) { return result; } throw RubyExceptions.CreateTypeError(String.Format("{0}#allocate should return IList", cls.Name)); } + internal static IEnumerable<Int32>/*!*/ ReverseEnumerateIndexes(IList/*!*/ collection) { + for (int originalSize = collection.Count, i = originalSize - 1; i >= 0; i--) { + yield return i; + if (collection.Count < originalSize) { + i = originalSize - (originalSize - collection.Count); + originalSize = collection.Count; + } + } + } + #endregion #region initialize_copy, replace, clear, to_a, to_ary [RubyMethod("replace")] [RubyMethod("initialize_copy", RubyMethodAttributes.PrivateInstance)] public static IList/*!*/ Replace(RubyContext/*!*/ context, IList/*!*/ self, [NotNull, DefaultProtocol]IList/*!*/ other) { RubyUtils.RequiresNotFrozen(context, self); self.Clear(); AddRange(self, other); return self; } [RubyMethod("clear")] @@ -1022,37 +1032,33 @@ namespace IronRuby.Builtins { return Index(equals, self, item) != null; } [RubyMethod("index")] public static object Index(BinaryOpStorage/*!*/ equals, IList/*!*/ self, object item) { for (int i = 0; i < self.Count; ++i) { if (Protocols.IsEqual(equals, self[i], item)) { return i; } } return null; } [RubyMethod("rindex")] public static object ReverseIndex(BinaryOpStorage/*!*/ equals, IList/*!*/ self, object item) { - for (int originalSize = self.Count, i = originalSize - 1; i >= 0; i--) { - if (Protocols.IsEqual(equals, self[i], item)) { - return i; - } - if (self.Count < originalSize) { - i = originalSize - i - 1 + self.Count; - originalSize = self.Count; + foreach (int index in IListOps.ReverseEnumerateIndexes(self)) { + if (Protocols.IsEqual(equals, self[index], item)) { + return index; } } return null; } #endregion #region indexes, indices, values_at [RubyMethod("indexes")] [RubyMethod("indices")] public static object Indexes(ConversionStorage<int>/*!*/ fixnumCast, CallSiteStorage<Func<CallSite, RubyClass, object>>/*!*/ allocateStorage, IList/*!*/ self, [NotNull]params object[]/*!*/ values) { fixnumCast.Context.ReportWarning("Array#indexes and Array#indices are deprecated; use Array#values_at");
Daniele Alessandri
2009-Jun-24 20:29 UTC
[Ironruby-core] Review: fixes for Array#rindex and Array#reverse_each
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 at gmail.com> wrote:> 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 > > > On Thu, Apr 23, 2009 at 22:08, Jim Deville <jdeville at microsoft.com> wrote: >> 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 >> >>> -----Original Message----- >>> From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core- >>> bounces at rubyforge.org] On Behalf Of Daniele Alessandri >>> Sent: Thursday, April 23, 2009 12:59 PM >>> To: ironruby-core at rubyforge.org >>> Subject: [Ironruby-core] 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/d2b18f5d01a49cb62a2ea0c205e1cf123 >>> 3ac94e0 >>> >>> >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 >>> >>> -- >>> Daniele Alessandri >>> http://www.clorophilla.net/blog/ >>> http://twitter.com/JoL1hAHN >> _______________________________________________ >> Ironruby-core mailing list >> Ironruby-core at rubyforge.org >> http://rubyforge.org/mailman/listinfo/ironruby-core >> > > > > -- > Daniele Alessandri > http://www.clorophilla.net/blog/ > http://twitter.com/JoL1hAHN >-- Daniele Alessandri http://www.clorophilla.net/blog/ http://twitter.com/JoL1hAHN
Jim Deville
2009-Jun-24 21:42 UTC
[Ironruby-core] Review: fixes for Array#rindex and Array#reverse_each
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> -----Original Message----- > From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core- > bounces at rubyforge.org] On Behalf Of Daniele Alessandri > Sent: Wednesday, June 24, 2009 1:30 PM > To: ironruby-core at rubyforge.org > Subject: Re: [Ironruby-core] Review: fixes for Array#rindex and > Array#reverse_each > > 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 at gmail.com> > wrote: > > 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/76db817d1766b788f995afb02e12c5 > a7 > > 2955c77f > > > > 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/e5497cf87fc479b2bf2ca0b812d9793 > 5 > > 4a86f44c > > > > See also the attached diff. > > > > Thanks, > > Daniele > > > > > > On Thu, Apr 23, 2009 at 22:08, Jim Deville <jdeville at microsoft.com> wrote: > >> 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 > >> > >>> -----Original Message----- > >>> From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core- > >>> bounces at rubyforge.org] On Behalf Of Daniele Alessandri > >>> Sent: Thursday, April 23, 2009 12:59 PM > >>> To: ironruby-core at rubyforge.org > >>> Subject: [Ironruby-core] 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/d2b18f5d01a49cb62a2ea0c205e1cf > >>> 123 > >>> 3ac94e0 > >>> > >>> >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 > >>> > >>> -- > >>> Daniele Alessandri > >>> http://www.clorophilla.net/blog/ > >>> http://twitter.com/JoL1hAHN > >> _______________________________________________ > >> Ironruby-core mailing list > >> Ironruby-core at rubyforge.org > >> http://rubyforge.org/mailman/listinfo/ironruby-core > >> > > > > > > > > -- > > Daniele Alessandri > > http://www.clorophilla.net/blog/ > > http://twitter.com/JoL1hAHN > > > > > > -- > Daniele Alessandri > http://www.clorophilla.net/blog/ > http://twitter.com/JoL1hAHN > _______________________________________________ > Ironruby-core mailing list > Ironruby-core at rubyforge.org > http://rubyforge.org/mailman/listinfo/ironruby-core
Daniele Alessandri
2009-Jun-24 23:23 UTC
[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 at 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 > > > > -----Original Message----- > > From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core- > > bounces at rubyforge.org] On Behalf Of Daniele Alessandri > > Sent: Wednesday, June 24, 2009 1:30 PM > > To: ironruby-core at rubyforge.org > > Subject: Re: [Ironruby-core] Review: fixes for Array#rindex and > > Array#reverse_each > > > > 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 at gmail.com> > > wrote: > > > 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/76db817d1766b788f995afb02e12c5 > > a7 > > > 2955c77f > > > > > > 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/e5497cf87fc479b2bf2ca0b812d9793 > > 5 > > > 4a86f44c > > > > > > See also the attached diff. > > > > > > Thanks, > > > Daniele > > > > > > > > > On Thu, Apr 23, 2009 at 22:08, Jim Deville <jdeville at microsoft.com> > wrote: > > >> 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 > > >> > > >>> -----Original Message----- > > >>> From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core- > > >>> bounces at rubyforge.org] On Behalf Of Daniele Alessandri > > >>> Sent: Thursday, April 23, 2009 12:59 PM > > >>> To: ironruby-core at rubyforge.org > > >>> Subject: [Ironruby-core] 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/d2b18f5d01a49cb62a2ea0c205e1cf > > >>> 123 > > >>> 3ac94e0 > > >>> > > >>> >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 > > >>> > > >>> -- > > >>> Daniele Alessandri > > >>> http://www.clorophilla.net/blog/ > > >>> http://twitter.com/JoL1hAHN > > >> _______________________________________________ > > >> Ironruby-core mailing list > > >> Ironruby-core at rubyforge.org > > >> http://rubyforge.org/mailman/listinfo/ironruby-core > > >> > > > > > > > > > > > > -- > > > Daniele Alessandri > > > http://www.clorophilla.net/blog/ > > > http://twitter.com/JoL1hAHN > > > > > > > > > > > -- > > Daniele Alessandri > > http://www.clorophilla.net/blog/ > > http://twitter.com/JoL1hAHN > > _______________________________________________ > > Ironruby-core mailing list > > Ironruby-core at rubyforge.org > > http://rubyforge.org/mailman/listinfo/ironruby-core > _______________________________________________ > Ironruby-core mailing list > Ironruby-core at rubyforge.org > http://rubyforge.org/mailman/listinfo/ironruby-core >-- Daniele Alessandri http://www.clorophilla.net/blog/ http://twitter.com/JoL1hAHN -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20090625/8753fbac/attachment.html>
Jim Deville
2009-Jun-24 23:29 UTC
[Ironruby-core] Review: fixes for Array#rindex and Array#reverse_each
Should be fine. I don?t plan on doing a pull until Friday. From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Daniele Alessandri Sent: Wednesday, June 24, 2009 4:23 PM To: ironruby-core at 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 at microsoft.com<mailto:jdeville at 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> -----Original Message----- > From: ironruby-core-bounces at rubyforge.org<mailto:ironruby-core-bounces at rubyforge.org> [mailto:ironruby-core-<mailto:ironruby-core-> > bounces at rubyforge.org<mailto:bounces at rubyforge.org>] On Behalf Of Daniele Alessandri > Sent: Wednesday, June 24, 2009 1:30 PM > To: ironruby-core at rubyforge.org<mailto:ironruby-core at rubyforge.org> > Subject: Re: [Ironruby-core] Review: fixes for Array#rindex and > Array#reverse_each > > 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 at gmail.com<mailto:suppakilla at gmail.com>> > wrote: > > 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/76db817d1766b788f995afb02e12c5 > a7 > > 2955c77f > > > > 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/e5497cf87fc479b2bf2ca0b812d9793 > 5 > > 4a86f44c > > > > See also the attached diff. > > > > Thanks, > > Daniele > > > > > > On Thu, Apr 23, 2009 at 22:08, Jim Deville <jdeville at microsoft.com<mailto:jdeville at microsoft.com>> wrote: > >> 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 > >> > >>> -----Original Message----- > >>> From: ironruby-core-bounces at rubyforge.org<mailto:ironruby-core-bounces at rubyforge.org> [mailto:ironruby-core-<mailto:ironruby-core-> > >>> bounces at rubyforge.org<mailto:bounces at rubyforge.org>] On Behalf Of Daniele Alessandri > >>> Sent: Thursday, April 23, 2009 12:59 PM > >>> To: ironruby-core at rubyforge.org<mailto:ironruby-core at rubyforge.org> > >>> Subject: [Ironruby-core] 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/d2b18f5d01a49cb62a2ea0c205e1cf > >>> 123 > >>> 3ac94e0 > >>> > >>> >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 > >>> > >>> -- > >>> Daniele Alessandri > >>> http://www.clorophilla.net/blog/ > >>> http://twitter.com/JoL1hAHN > >> _______________________________________________ > >> Ironruby-core mailing list > >> Ironruby-core at rubyforge.org<mailto:Ironruby-core at rubyforge.org> > >> http://rubyforge.org/mailman/listinfo/ironruby-core > >> > > > > > > > > -- > > Daniele Alessandri > > http://www.clorophilla.net/blog/ > > http://twitter.com/JoL1hAHN > > > > > > -- > Daniele Alessandri > http://www.clorophilla.net/blog/ > http://twitter.com/JoL1hAHN > _______________________________________________ > Ironruby-core mailing list > Ironruby-core at rubyforge.org<mailto:Ironruby-core at rubyforge.org> > http://rubyforge.org/mailman/listinfo/ironruby-core_______________________________________________ Ironruby-core mailing list Ironruby-core at rubyforge.org<mailto:Ironruby-core at rubyforge.org> http://rubyforge.org/mailman/listinfo/ironruby-core -- Daniele Alessandri http://www.clorophilla.net/blog/ http://twitter.com/JoL1hAHN -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20090624/338ace9a/attachment.html>
Daniele Alessandri
2009-Jun-25 20:48 UTC
[Ironruby-core] Review: fixes for Array#rindex and Array#reverse_each
Just merged and pushed to my remote repository. On Thu, Jun 25, 2009 at 01:29, Jim Deville<jdeville at microsoft.com> wrote:> Should be fine. I don?t plan on doing a pull until Friday. > > > > From: ironruby-core-bounces at rubyforge.org > [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Daniele Alessandri > Sent: Wednesday, June 24, 2009 4:23 PM > > To: ironruby-core at 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 at 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 > >> -----Original Message----- >> From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core- >> bounces at rubyforge.org] On Behalf Of Daniele Alessandri > >> Sent: Wednesday, June 24, 2009 1:30 PM >> To: ironruby-core at rubyforge.org > >> Subject: Re: [Ironruby-core] Review: fixes for Array#rindex and >> Array#reverse_each >> >> 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 at gmail.com> >> wrote: >> > 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/76db817d1766b788f995afb02e12c5 >> a7 >> > 2955c77f >> > >> > 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/e5497cf87fc479b2bf2ca0b812d9793 >> 5 >> > 4a86f44c >> > >> > See also the attached diff. >> > >> > Thanks, >> > Daniele >> > >> > >> > On Thu, Apr 23, 2009 at 22:08, Jim Deville <jdeville at microsoft.com> >> > wrote: >> >> 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 >> >> >> >>> -----Original Message----- >> >>> From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core- >> >>> bounces at rubyforge.org] On Behalf Of Daniele Alessandri >> >>> Sent: Thursday, April 23, 2009 12:59 PM >> >>> To: ironruby-core at rubyforge.org >> >>> Subject: [Ironruby-core] 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/d2b18f5d01a49cb62a2ea0c205e1cf >> >>> 123 >> >>> 3ac94e0 >> >>> >> >>> >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 >> >>> >> >>> -- >> >>> Daniele Alessandri >> >>> http://www.clorophilla.net/blog/ >> >>> http://twitter.com/JoL1hAHN >> >> _______________________________________________ >> >> Ironruby-core mailing list >> >> Ironruby-core at rubyforge.org >> >> http://rubyforge.org/mailman/listinfo/ironruby-core >> >> >> > >> > >> > >> > -- >> > Daniele Alessandri >> > http://www.clorophilla.net/blog/ >> > http://twitter.com/JoL1hAHN >> > >> >> >> >> -- >> Daniele Alessandri >> http://www.clorophilla.net/blog/ >> http://twitter.com/JoL1hAHN >> _______________________________________________ >> Ironruby-core mailing list >> Ironruby-core at rubyforge.org >> http://rubyforge.org/mailman/listinfo/ironruby-core > _______________________________________________ > Ironruby-core mailing list > Ironruby-core at rubyforge.org > http://rubyforge.org/mailman/listinfo/ironruby-core > > > -- > Daniele Alessandri > http://www.clorophilla.net/blog/ > http://twitter.com/JoL1hAHN > > _______________________________________________ > Ironruby-core mailing list > Ironruby-core at rubyforge.org > http://rubyforge.org/mailman/listinfo/ironruby-core > >-- Daniele Alessandri http://www.clorophilla.net/blog/ http://twitter.com/JoL1hAHN