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

Add append_array() method to Array class #43398

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Nov 8, 2020

Closes godotengine/godot-proposals#1718

Vector class (used internally by Array) already had append_array method, so this PR pretty much only exposes it.

@KoBeWi KoBeWi added enhancement feature proposal topic:core cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Nov 8, 2020
@KoBeWi KoBeWi added this to the 4.0 milestone Nov 8, 2020
@KoBeWi KoBeWi requested a review from a team as a code owner November 8, 2020 20:10
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

This is also consistent with Packed*Arrays which have append_array.

@akien-mga
Copy link
Member

In the proposal @Zylann raises a good point about potentially missing the opportunity to optimize the += operator so that it doesn't translate to a = a + b but actually does the same as append_array internally:

godotengine/godot-proposals#1718 (comment)

This shouldn't prevent merging this for consistency with Packed*Array but we should likely discuss it first nevertheless (CC @vnen @reduz).

@akien-mga akien-mga requested review from reduz and vnen November 10, 2020 09:01
@vnen
Copy link
Member

vnen commented Nov 10, 2020

In the proposal @Zylann raises a good point about potentially missing the opportunity to optimize the += operator so that it doesn't translate to a = a + b but actually does the same as append_array internally:

That might be a bit tricky. For this to work well I think Variant should provide the "assign with operation" operator as well. From GDScript it's hard to tell if the code isn't typed.

@akien-mga
Copy link
Member

Further comments from IRC:

13:22 <Akien> reduz: vnen: WDYT about #43398 ? I think it's fine to add the method since Packed*Arrays have it, but should we also do something to improve the `+=` operator to avoid doing a copy?
13:24 <vnen> Akien: oh, I just commented there
13:25 <vnen> I think we need the += operator in variant itself for this to work
13:33 <reduz> Akien, vnen: appending arrays is not really a high performance use case in general, so probably not worth it. I think if the function is exposed its better than having to implement += operators
13:36 <Akien> String::operator+=() does seem to mutate the original String though, so container-wise it's a bit inconsistent that Arrays can't do it.
13:36 <vnen> yeah, users should know better what to use in this case
13:36 <Akien> But yeah having the explicit method seems a good step already.
13:37 <vnen> if you use from GDScript it won't call the C++ operator, it will just create copy

So changing += wouldn't be that simple. Let's merge this as a good first step to let users optimize their allocations.

@akien-mga akien-mga merged commit 03ae26b into godotengine:master Nov 10, 2020
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the add_an_array_to_another_array_but_with_a_method branch November 10, 2020 12:55
@akien-mga
Copy link
Member

Cherry-picked for 3.2.4.

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.

Add an extend() method in Array
3 participants