Skip to content

Add support for range based loops for Array #1717

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

Merged

Conversation

tomfull123
Copy link
Contributor

@tomfull123 tomfull123 commented Feb 22, 2025

Fixes #1710
This PR adds support for range based loops for the TypedArray class.
Allowing syntax such as:

TypedArray<int> elements;
for (const auto& e : elements) {}

I wanted to get proper type inference for the element, but I had issues getting the Variant type to cast properly.

@tomfull123 tomfull123 requested a review from a team as a code owner February 22, 2025 15:05
@dsnopek
Copy link
Collaborator

dsnopek commented Mar 13, 2025

Thanks!

However, one of godot-cpp's core design goals is to be compatible with Godot's internal APIs, and this differs a little bit from them:

  • I don't think we should publicly expose ptr() or ptrw() because Array/TypedArray don't have them in Godot
  • These iterators should be on Array rather than TypedArray, because that's where they are in Godot, and that would also allow folks to use range-based loops an Arrays, which would be nice

@dsnopek dsnopek added the enhancement This is an enhancement on the current functionality label Mar 13, 2025
@dsnopek dsnopek added this to the 4.x milestone Mar 13, 2025
@tomfull123 tomfull123 changed the title Add support for range based loops for TypedArray Add support for range based loops for Array Mar 18, 2025
@tomfull123
Copy link
Contributor Author

tomfull123 commented Mar 18, 2025

Changed the PR to now add the iterator to Array instead of TypedArray, I can make the ptr() and ptrw() methods private, but they are public in the PackedArray classes, so it would be consistent with those, but maybe its better for them to have those methods private too, what do you think? @dsnopek

I think ptr() and ptrw() should be made private in all the PackedArray classes too, to match Godot, I can put in a PR for that too.

@dsnopek
Copy link
Collaborator

dsnopek commented Mar 19, 2025

Thanks!

Changed the PR to now add the iterator to Array instead of TypedArray, I can make the ptr() and ptrw() methods private, but they are public in the PackedArray classes, so it would be consistent with those, but maybe its better for them to have those methods private too, what do you think?

The goal isn't to be internally consistent among the classes in godot-cpp, but to be consistent with the API of Godot.

In Godot, the PackedArray classes have public ptr() and ptrw() methods, but Array/TypedArray do not, so we should do the same in godot-cpp. If we wanted to have those methods on Array/TypedArray, we'd need to make a PR to add them to Godot first.

Also, lots of extensions are already using ptr() and ptrw() on the PackedArray classes (including some I maintain :-)) so we really couldn't remove them now anyway

@tomfull123
Copy link
Contributor Author

tomfull123 commented Mar 19, 2025

@dsnopek cool, the Array's ptr() and ptrw() methods are now private

@tomfull123 tomfull123 requested a review from dsnopek March 26, 2025 12:36
@tomfull123
Copy link
Contributor Author

Is this PR waiting on anything? @dsnopek do you have any other concerns?

@dsnopek
Copy link
Collaborator

dsnopek commented Apr 1, 2025

Thanks, this is looking good to me now!

Only one last thing - this needs to be squashed to a single commit per Godot's git workflow: https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

(This applies to all your PRs, actually - sorry that I didn't notice earlier that you were making multiple commits)

@tomfull123 tomfull123 force-pushed the typed-array-range-based-loop-support branch from 4eca894 to a2c37f8 Compare April 1, 2025 19:48
@tomfull123
Copy link
Contributor Author

Ahh no worries, I should have read the PR guidelines more thoroughly, should be good to go now

@dsnopek dsnopek merged commit da064d8 into godotengine:master Apr 2, 2025
11 checks passed
@tomfull123 tomfull123 deleted the typed-array-range-based-loop-support branch April 2, 2025 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.4 enhancement This is an enhancement on the current functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Godot-cpp Array API does not feature begin/end functions
2 participants