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 <jdevi...@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-boun...@rubyforge.org [mailto:ironruby-core-
>> boun...@rubyforge.org] On Behalf Of Daniele Alessandri
>> Sent: Thursday, April 23, 2009 12:59 PM
>> To: ironruby-core@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@rubyforge.org
> http://rubyforge.org/mailman/listinfo/ironruby-core
>
--
Daniele Alessandri
http://www.clorophilla.net/blog/
http://twitter.com/JoL1hAHN
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");
_______________________________________________
Ironruby-core mailing list
Ironruby-core@rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core
