Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discrepancy in #[](start, count) with negative start in array-like collections #14775

Open
ysbaddaden opened this issue Jul 3, 2024 · 2 comments

Comments

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Jul 3, 2024

While trying get the last 3 bytes in a slice, I noticed that bytes[-3, 3] failed unexpectedly. Digging a bit, I noticed that there is a discrepancy in how [](start, count) with a negative start behaves in the different array-like collections:

  • Array: returns count entries starting from the last start entries (expected);
  • Deque: not implemented;
  • Slice: raises an Index out of bounds exception (huh?).

Given that [](index) with a negative index is behaving the same in all these types (return item at index starting from the end), I'd expect [](count, start) to behave the same, too.

The method isn't implemented for StaticArray but it's likely deliberate (aka not possible). It may not make much sense for Deque either (though possible), but Slice was surprising.

@HertzDevil
Copy link
Contributor

For Slice there is also #12617

@straight-shoota
Copy link
Member

straight-shoota commented Jul 3, 2024

I suppose we should utilize Indexable.normalize_start_and_count in Slice#[]?. The purpose of this helper method is exactly to synchronize this behaviour across implementations.

We actually use it already in Slice#fill. However, there is also a comment mentioning the restricted behaviour of Slice#[] regarding count.

crystal/src/slice.cr

Lines 452 to 456 in be46ba2

def fill(value : T, start : Int, count : Int) : self
# since `#[]` requires exactly *count* elements but we allow fewer here, we
# must normalize the indices beforehand
start, count = normalize_start_and_count(start, count)
self[start, count].fill(value)

And yeah, I find the implicit clamping of count can be a bit surprising. I would expect the result of slice[i, 8] to have 8 elements, not "up to 8" (Math.min(slice.size - i, 8)). I'd probably expect the same for Array as well. 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants