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

[Core] Add iteration support to Array #86518

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Dec 26, 2023

Added simple iteration support for Array, using the same method as the subscript operator, added the read-only part to the constant one as well as well for parity with that operator

Many, many, possible uses across the codebase but kept this restricted to a few cases, considering doing one of my sweeping PRs to improve use of foreach generally not just for this new addition, but will have to wait for right now

Put some of the methods in variant.h as they depend on Variant being declared and wanted them to be inlineable

Not tagging this as Performance as any improvement would be minimal, but it does, theoretically, carry a minor performance improvement as it avoids doing COW checks on each access, which would most of the time be a wasted check as the Vector is almost always not shared yet the mutation check is always performed

Will squash the added cases once decided in case some additional "good to add" cases are suggested

@AThousandShips
Copy link
Member Author

I've been investigating an equivalent addition to Dictionary but it's a bit more complex due to the internal data, but will open one if that one pans out as well

@dsnopek
Copy link
Contributor

dsnopek commented Dec 30, 2023

Thanks, this looks great to me!

Will squash the added cases once decided in case some additional "good to add" cases are suggested

I think having only these few cases is fine, in fact, reviewing those changes is trickier (at least for me) than the added iterator code, so in my opinion "less is more" here :-)

@Calinou
Copy link
Member

Calinou commented Jan 1, 2024

I think once we have added this to List, we'll be able to use for-range looping almost everywhere where it makes sense. (That said, many existing uses of List should be replaced with other faster container types like Vector and LocalVector.)

@AThousandShips
Copy link
Member Author

List already supports this 🙂

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me!

I attempted to really scrutinize the changes to existing code to start using the iterator, and didn't notice anything that seemed like it would change behavior.

I'm not on the core team but decided to be bold and hit "approve" - I really want to be able to use this myself :-)

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Apr 10, 2024
@akien-mga akien-mga merged commit 7670b81 into godotengine:master Apr 10, 2024
16 checks passed
@AThousandShips AThousandShips deleted the array_iter branch April 10, 2024 15:53
@akien-mga
Copy link
Member

Thanks!

@AThousandShips
Copy link
Member Author

Thank you!

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.

4 participants