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 Array.resize method #697

Merged
merged 1 commit into from Apr 2, 2018

Conversation

Projects
None yet
2 participants
@bendmorris
Copy link
Contributor

bendmorris commented Feb 23, 2018

From HaxeFoundation/haxe#6173

Adds a resize method to Array. Didn't implement this for cppia as I'm not familiar with that target.

@hughsando

This comment has been minimized.

Copy link
Member

hughsando commented Feb 27, 2018

This is currently done via:

using cpp.NativeArray;

...
array.setSize(1234);

How were you intending to call 'resize' - dynamically or untyped?

@bendmorris

This comment has been minimized.

Copy link
Contributor

bendmorris commented Feb 27, 2018

See the linked Haxe PR - intention is to add this as a standard Array method.

@bendmorris

This comment has been minimized.

Copy link
Contributor

bendmorris commented Mar 3, 2018

@hughsando - any issues with this implementation? Is there a better way to do it?

@bendmorris

This comment has been minimized.

Copy link
Contributor

bendmorris commented Apr 1, 2018

@hughsando - bump. The goal here is to add Array.resize for all targets, and unless that's possible without modifying hxcpp, this PR would need to go in first. Would appreciate a response.

@hughsando

This comment has been minimized.

Copy link
Member

hughsando commented Apr 2, 2018

I guess my main issue is to do with the removal or wrapping of the __SetSize function.
If resize is going to be official, I think all the setSize calls should be replaced with resize calls, and there should be some setSize stubs to call resize for backwards compatibility.
Also, I think the VirtualArray version is going to need empty array condition that is in the __SetSize function - although this is not an issue if the set size function is renamed.
Similarly for cppia - will need a solution here, or the tests will fail - it will be very similar to the SetSizeExact that is already in there.
Other questions are - the return value - void or 'this' ?
And, is it __SetSizeExact or __SetSize - the difference being the exact version will free memory if you shrink the array, whereas SetSize will not.
None of these issues is that big or difficult - they are pretty trivial really. But it was enough to stop me just hitting the merge button, and then I kinda forgot about the issue.

@hughsando

This comment has been minimized.

Copy link
Member

hughsando commented Apr 2, 2018

Ok, I see from the other targets that no memory is released, and it returns void, so it is for __SetSize

@hughsando hughsando merged commit 837613f into HaxeFoundation:master Apr 2, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bendmorris

This comment has been minimized.

Copy link
Contributor

bendmorris commented Apr 2, 2018

Thanks Hugh. I'll look into cppia next.

@hughsando

This comment has been minimized.

Copy link
Member

hughsando commented Apr 3, 2018

I put the changes into cppia, but have not really tested them.

@bendmorris

This comment has been minimized.

Copy link
Contributor

bendmorris commented Apr 3, 2018

Awesome - my haxe PR includes tests, so I'll get back to you if there are any issues.

@skial skial referenced this pull request Apr 4, 2018

Closed

Haxe Roundup 426 #494

1 of 1 task complete

@bendmorris bendmorris deleted the bendmorris:array-resize branch Apr 20, 2018

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