-
Notifications
You must be signed in to change notification settings - Fork 7.8k
[WIP][RFC]Spread Operator in Array #3640
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
Thanks for working on this! From a quick look the patch looks reasonable. One somewhat contentious issue that came up the last time we discussed this in room 11 is the behavior of string keys. This PR implements the same semantics as array_merge. An alternative would be to always discard keys and append elements. Another would be to error on string keys (that's what argument unpacking does). I think it would be good to discuss this question in the RFC and provide a rationale why the behavior was chosen. |
great work, i was going to propose something similar to this before but never get to it. |
A question: will this be supported?
|
c82cb1b
to
62eb3ac
Compare
hi @Llbe , this is supposed to be supported since it's a constant expression. Unfortunately I got a core dump when I try to run your example. I'll look into it later. thanks. update: I cannot reproduce the crash on my windows. I'll finish updating the implement first then go back to the core dump. |
@jhdxr: another question (sorry I'm not on internals yet), the RFC doesn't mention how empty arrays are handled. I assume that |
Yes, you are right. |
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.
Some changes in opcache are likely going to be needed. A good starting point is to look for ADD_ARRAY_ELEMENT uses in ext/opcache and see if the new UNPACK opcode needs similar treatment.
5507d8b
to
9d91a88
Compare
de3765e
to
fa5be85
Compare
Question: will this support destructuring assignment as well?
That's pretty neat for Haskell-style list splitting like |
@datashaman no, spread operator with |
fa5be85
to
7567d3d
Compare
Do you need help with opcache support? |
@@ -7234,6 +7346,7 @@ ZEND_VM_HANDLER(149, ZEND_HANDLE_EXCEPTION, ANY, ANY) | |||
if (throw_op->result_type & (IS_VAR | IS_TMP_VAR)) { | |||
switch (throw_op->opcode) { | |||
case ZEND_ADD_ARRAY_ELEMENT: | |||
case ZEND_ADD_ARRAY_UNPACK: |
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.
zend_opcode.c also needs updates (add ADD_ARRAY_UNPACK next to ADD_ARRAY_ELEMENT).
@nikic Yes, it will be very helpful. thank you. I have another branch used for developing and testing with opcache, but the progress is very slow. maybe you can just ignore this one (:з」∠) |
Result will be freed by exception handling, don't destroy it ourselves.
And also fix one discrepancy in how many warnings you get when the unpack is AST evaluated.
And tweak error message for grammar.
Opcache support is done, and I've also added a few more bits of test coverage. |
thank you @nikic |
Maybe it makes sense to use a different syntax instead of |
this is the implement for RFC Spread Operator in Array Expression
both the RFC and this PR are still WIP, but since most of the implement has been done, I'd like to create PR first. it's no harm to give people more time to review this, isn't it?
todo list:
list()
, since it also usearray_pair