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: Relax slice bound checks to properly handle negative indices #56668

Merged

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Jan 10, 2022

The same is done for Vector (and thus Packed*Array).

begin and end can now take any value and will be clamped to
[-size(), size()]. Negative values are a shorthand for indexing the array
from the last element upward.

end is given a default INT_MAX value (which will be clamped to size())
so that the end parameter can be omitted to go from begin to the max size
of the array.

This makes slice works similarly to numpy's and JavaScript's.

Fixes #56661.


TODOs:

  • Write docs for Packed*Arrays
  • Update/add more unit tests for slice
  • Review C# and GDNative counterparts, if any

core/templates/vector.h Outdated Show resolved Hide resolved
@akien-mga akien-mga force-pushed the array-slice-nicer-bound-checks branch 4 times, most recently from 33c7f57 to 3ecd49f Compare January 10, 2022 21:28
The same is done for `Vector` (and thus `Packed*Array`).

`begin` and `end` can now take any value and will be clamped to
`[-size(), size()]`. Negative values are a shorthand for indexing the array
from the last element upward.

`end` is given a default `INT_MAX` value (which will be clamped to `size()`)
so that the `end` parameter can be omitted to go from `begin` to the max size
of the array.

This makes `slice` works similarly to numpy's and JavaScript's.
@akien-mga akien-mga force-pushed the array-slice-nicer-bound-checks branch from 3ecd49f to c6cefb1 Compare January 10, 2022 21:46
@akien-mga
Copy link
Member Author

@godotengine/mono I don't see any reference to Array.slice() in the Godot glue, is that expected?

@akien-mga akien-mga marked this pull request as ready for review January 10, 2022 22:10
@akien-mga akien-mga requested review from a team as code owners January 10, 2022 22:10
@akien-mga
Copy link
Member Author

This should be good to review. It works fine, the main question is whether we're fine with relaxing these checks and using INT_MAX as default value. I used INT_MAX instead of INT32_MAX or INT64_MAX as array/vector indices use int, but since GDScript int is explicitly int64_t there's a somewhat weird inconsistency here (but that goes through the whole codebase, aside from critical paths which have been improved recently like for FileAccess).

@akien-mga akien-mga merged commit 9912492 into godotengine:master Jan 18, 2022
@akien-mga akien-mga deleted the array-slice-nicer-bound-checks branch January 18, 2022 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array.slice() negative end inconsistent with other languages (off by one)
5 participants