Add support for negative start index in Slice#[start, count] (#14778)

Allows `slice[-3, 3]` to return the last 3 elements of the slice, for example, similar to how Array#[-start, count] behaves, with the difference that Slice returns exactly *count* elements, while Array returns up to *count* elements. Introduces a couple changes: - negative start now returns from the end of the slice instead of returning nil/raising IndexError - negative count now raises ArgumentError instead of returning nil/raising IndexError I believe the current behavior is buggy (unexpected result, underspecified documentation), but this can be considered a breaking change.
crystal-lang · Aug 7, 2024 · d738ac9 · d738ac9
1 parent ac2ecb0
commit d738ac9
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 12 deletions.
diff --git a/spec/std/slice_spec.cr b/spec/std/slice_spec.cr
@@ -104,25  104,34 @@ describe "Slice" do
 
   it "does []? with start and count" do
     slice = Slice.new(4) { |i| i   1 }
 
     slice1 = slice[1, 2]?
     slice1.should_not be_nil
     slice1 = slice1.not_nil!
     slice1.size.should eq(2)
     slice1.to_unsafe.should eq(slice.to_unsafe   1)
     slice1[0].should eq(2)
     slice1[1].should eq(3)
 
-    slice[-1, 1]?.should be_nil
     slice2 = slice[-1, 1]?
     slice2.should_not be_nil
     slice2 = slice2.not_nil!
     slice2.size.should eq(1)
     slice2.to_unsafe.should eq(slice.to_unsafe   3)
 
     slice[3, 2]?.should be_nil
     slice[0, 5]?.should be_nil
-    slice[3, -1]?.should be_nil
     expect_raises(ArgumentError, "Negative count: -1") { slice[3, -1]? }
   end
 
   it "does []? with range" do
     slice = Slice.new(4) { |i| i   1 }
 
     slice1 = slice[1..2]?
     slice1.should_not be_nil
     slice1 = slice1.not_nil!
     slice1.size.should eq(2)
     slice1.to_unsafe.should eq(slice.to_unsafe   1)
     slice1[0].should eq(2)
     slice1[1].should eq(3)
 
@@ -134,15  143,20 @@ describe "Slice" do
 
   it "does [] with start and count" do
     slice = Slice.new(4) { |i| i   1 }
 
     slice1 = slice[1, 2]
     slice1.size.should eq(2)
     slice1.to_unsafe.should eq(slice.to_unsafe   1)
     slice1[0].should eq(2)
     slice1[1].should eq(3)
 
-    expect_raises(IndexError) { slice[-1, 1] }
     slice2 = slice[-1, 1]
     slice2.size.should eq(1)
     slice2.to_unsafe.should eq(slice.to_unsafe   3)
 
     expect_raises(IndexError) { slice[3, 2] }
     expect_raises(IndexError) { slice[0, 5] }
-    expect_raises(IndexError) { slice[3, -1] }
     expect_raises(ArgumentError, "Negative count: -1") { slice[3, -1] }
   end
 
   it "does empty?" do
@@ -659,6  673,7 @@ describe "Slice" do
     subslice = slice[2..4]
     subslice.read_only?.should be_false
     subslice.size.should eq(3)
     subslice.to_unsafe.should eq(slice.to_unsafe   2)
     subslice.should eq(Slice.new(3) { |i| i   3 })
   end
 

diff --git a/src/slice.cr b/src/slice.cr
@@ -222,35  222,49 @@ struct Slice(T)
   end
 
   # Returns a new slice that starts at *start* elements from this slice's start,
-  # and of *count* size.
   # and of exactly *count* size.
   #
   # Negative *start* is added to `#size`, thus it's treated as index counting
   # from the end of the array, `-1` designating the last element.
   #
   # Raises `ArgumentError` if *count* is negative.
   # Returns `nil` if the new slice falls outside this slice.
   #
   # ```
   # slice = Slice.new(5) { |i| i   10 }
   # slice # => Slice[10, 11, 12, 13, 14]
   #
-  # slice[1, 3]?  # => Slice[11, 12, 13]
-  # slice[1, 33]? # => nil
   # slice[1, 3]?   # => Slice[11, 12, 13]
   # slice[1, 33]?  # => nil
   # slice[-3, 2]?  # => Slice[12, 13]
   # slice[-3, 10]? # => nil
   # ```
   def []?(start : Int, count : Int) : Slice(T)?
-    return unless 0 <= start <= @size
-    return unless 0 <= count <= @size - start
     # we skip the calculated count because the subslice must contain exactly
     # *count* elements
     start, _ = Indexable.normalize_start_and_count(start, count, size) { return }
     return unless count <= @size - start
 
     Slice.new(@pointer   start, count, read_only: @read_only)
   end
 
   # Returns a new slice that starts at *start* elements from this slice's start,
-  # and of *count* size.
   # and of exactly *count* size.
   #
   # Negative *start* is added to `#size`, thus it's treated as index counting
   # from the end of the array, `-1` designating the last element.
   #
   # Raises `ArgumentError` if *count* is negative.
   # Raises `IndexError` if the new slice falls outside this slice.
   #
   # ```
   # slice = Slice.new(5) { |i| i   10 }
   # slice # => Slice[10, 11, 12, 13, 14]
   #
-  # slice[1, 3]  # => Slice[11, 12, 13]
-  # slice[1, 33] # raises IndexError
   # slice[1, 3]   # => Slice[11, 12, 13]
   # slice[1, 33]  # raises IndexError
   # slice[-3, 2]  # => Slice[12, 13]
   # slice[-3, 10] # raises IndexError
   # ```
   def [](start : Int, count : Int) : Slice(T)
     self[start, count]? || raise IndexError.new