-
Notifications
You must be signed in to change notification settings - Fork 399
feat(extgen): add support for arrays as parameters and return types #1724
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
Conversation
428a194 to
fe73d31
Compare
39fc120 to
e0379da
Compare
|
CI fixed and green 👍 |
withinboredom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exciting times! My only concern is that we're passing zvals. How does that impact GC on the PHP side? Do we need to update the ref-counts of the zval? For example, if I have the following code:
function foo() {
// set a global in the extension, the value will be gc'd upon return
set_my_ext_global([]);
}
foo();
var_dump(get_my_ext_global());In this case, we won't have a memory leak (the original [] gets destroyed), and we actually get a deep copy later? Or, is there a reference still to [] hanging around?
|
|
||
| isList := true | ||
| for i, k := range arr.keys { | ||
| if k.Type != PHPIntKey || k.Int != int64(i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this be an issue on 32bit machines where the keys will be 32 bits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be good, manipulating int64 on 32 bits system is just a bit slower. But we can also type it with int instead of int64, so it adapts depending on the system. Not sure which would be better, given that 32 bits system are becoming pretty rare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually came up on php-internals recently. It is less rare than you'd think. For example, wasm implementations and emulators on ios are 32 bit.
It shouldn't as the copy is stored in the Go-side (if we have a global var in the extension). To me, we don't need to update the refcount as long as we don't support references (it should be documented as well in the doc indeed). Also we use |
ef86b31 to
bee717a
Compare
|
All comments addressed, thanks for the review! |
bee717a to
a5fe94d
Compare
|
Thanks! |
This brings support for arrays in parameters and return types, and writing such code:
TODO: