-
Notifications
You must be signed in to change notification settings - Fork 7.9k
[WIP][RFC] spread operator in list #5000
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
You don't need to use the internal array pointer (and foreach doesn't use it either). |
Thanks. It's really nice to know foreach doesn't use it all now. Do you have any idea on how to deal with null in this case? |
I'm assuming that, for now, we will have to follow usual list() behavior and silently accept null. As the final implementation (presumably) will not be based on foreach, it shouldn't be a problem to support that, right? |
I prefer not to add new opcpde, so if possible I'd like to reuse foreach. and the reason I want to do so is they are doing very similar jobs so repeating them will make future work, like optimization more complex/redundant. |
opline->op2.var = get_temporary_variable(); | ||
//save var? | ||
//no need to deal with jmp, since those visited indexes are guaranteed to exist. | ||
} |
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 may skip different elements than were fetched before, if the indexes/order of the array don't match. For example list($a, ...$b) = [1 => 1, 0 => 0]
. Then $a = 0
, but this will skip 1
instead.
I think implementing this based on foreach will be fairly fragile / inefficient, and I'm also concerned that this will break a lot of our internal invariants, because it is writing to a temporary variable inside a loop. While it is possible to have multiple writes to the same temporary, this is subject to a number of invariants, and breaking them will likely cause issues for things like live ranges (exception handling) and temporary compaction. I would really prefer to add a new opcode for this. |
Noted. I'll implement it with a new opcode. |
This is a POC of spread operator in
list
. I haven't start working on the RFC and tests yet, but I got some questions during implementing this feature. so I decided to create this PR first so we can have some discussion.list
was a syntax sugar, I want to keep it as a syntax after adding support of spread operator.the reason I choose to implement it in this way is I don't want to any new opcode at this stage.
null
.Although we passed a RFC to deal with reading from null, we still have some arguments in PR(Fix #37676 : Add E_WARNING when using array-index on non valid container #2031). Nikita provided Throw notice on array offset access of null/bool/int/etc #4386 which excluded list in the implementation. (More background about this decision can be found in these two PR).
current this PR still generator warnings if spread operator runs into null, because
foreach
will generator warnings if it's used withnull
. I don't know if it's a good idea to keep this warning or omit it.2. internal pointer of arraypreviously
list
will not modify the array to be unpacked because all the index can be determined during compile time. unfortunately, spread operator is dealing with variable-length array that only can be processed during runtime. Thus internal pointer of the hashtable is used / modified.This should not be considered as a BC break since everything remains same as long as you don't use spread operator, but I'm still worring if this side-effect is something we can bear.