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

Array.slice() negative end inconsistent with other languages (off by one) #56661

Closed
akien-mga opened this issue Jan 10, 2022 · 1 comment · Fixed by #56668
Closed

Array.slice() negative end inconsistent with other languages (off by one) #56661

akien-mga opened this issue Jan 10, 2022 · 1 comment · Fixed by #56668

Comments

@akien-mga
Copy link
Member

akien-mga commented Jan 10, 2022

Godot version

4.0.dev (d746475)

System information

Linux, Mageia 9 x86_64

Issue description

Follow-up to PR #35901 and issue #53495.

The behavior of the negative end parameter for Array.slice() seems inconsistent with how it behaves in other languages. Our implementation uses:

        if (p_end < 0) {
                p_end += size() + 1;
        }

And this + 1 adds an inconsistency with other implementations.

GDScript

var arr = [1, 2, 3]
print(arr.slice(0, arr.size()))
print(arr.slice(0, -1))
print(arr.slice(0, -2))

prints:

[1, 2, 3]
[1, 2, 3]
[1, 2]

Python / numpy

import numpy
arr = numpy.array([1, 2, 3])
print(arr[0:])
print(arr[0:-1])
print(arr[0:-2])

prints:

[1 2 3]
[1 2]
[1]

JavaScript

arr = [1, 2, 3];
console.log(arr.slice(0))
console.log(arr.slice(0, -1))
console.log(arr.slice(0, -2))

prints:

[1, 2, 3]
[1, 2]
[1]

We can easily remove the +1 but then the question is how to make it easy to go from begin to the size of the original array without repeating arr.size().

Making the end argument optional like in JavaScript could be an option, but I'm not sure it's easily doable in GDScript. Aside from turning the int end into Variant end, we can't easily make it null or use another state to indicate the "go to size()" function. 0 would be a valid end for an empty slice, and -1 should behave like size() - 1 (wraps around).

And binding a slice(int begin) method is not possible either without polymorphism AFAIK.

Steps to reproduce

var arr = [1, 2, 3]
print(arr.slice(0, arr.size()))
print(arr.slice(0, -1))
print(arr.slice(0, -2))

Expected:

[1, 2, 3]
[1, 2]
[1]

but got:

[1, 2, 3]
[1, 2, 3]
[1, 2]

Minimal reproduction project

See above.

@akien-mga
Copy link
Member Author

After discussion with @KoBeWi and @timothyqiu, it seems like making the function more permissive on possible values for begin and end and giving end a default of INT_MAX could do the trick:

<KoBeWi> we could use something like INT_MAX for default argument. It's unlikely someone will need it
<timothyqiu> `arr.slice()` is also different from Python and JS slices when `end` is greater than the array size + 1. In Python/JS, it means the end of array. While in GDScript it fails.
<Akien> Hm if we combine both comments then `INT_MAX` sounds like an option.

I'll give this a try and make a draft PR, we'll see if it's a good idea.

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

Successfully merging a pull request may close this issue.

1 participant