-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Added default
as an expression in argument lists
#15437
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
base: master
Are you sure you want to change the base?
Conversation
098ef37
to
10cfffe
Compare
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.
Not commenting on things listed in the todos.
Patched match expression to work with the new grammar. Changed ZEND_FETCH_DEFAULT_ARG VM handler to use reflection_parameter_ptr.
Exposed get_parameter_default from reflection API.
Added exception handling for passing `default` to required parameters. Fixed default_value leak in ZEND_FETCH_DEFAULT_ARG handler.
Added compile-time exclusion for default in non-argument contexts.
fd34f8d
to
29855cc
Compare
if (op_array->opcodes[i].opcode == ZEND_FETCH_DEFAULT_ARG) { | ||
return; | ||
} | ||
} |
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.
As mentioned off-list, this should check only for opcodes within the current call (between fcall
and opline
), not the entire function. It would be nice to also skip nested functions, but that will require listing all init/do opcodes.
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.
Thanks for the note. I am still trying to figure out how to do this.
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.
Store fcall in a local var, loop, and bump until reaching opline.
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.
There is code for that in PHP already somewhere, and now also in Xdebug: xdebug/xdebug@551b203#diff-846d4897469701b1d805f3574922dc47b632fd189087a2589b2e01e848585a8bR378 — that should probably become a PHP API function.
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.
I'll be honest, I still have no idea what to do here, and no idea how the Xdebug code relates to this. Anyway, I'll deal with this if the feature gets accepted.
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.
for (zend_op *op = fcall + 1; op != opline; op++) {
// Don't inline calls with explicit default arguments.
if (op->opcode == ZEND_FETCH_DEFAULT_ARG) {
return;
}
}
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.
Thanks. Clearly I need to understand more about what oparrays, oplines and opcodes really are, and how they relate to each other. I tend to imagine that an oparray contains oplines which contain opcodes, but I still don't really know why oparray is even a thing, and when you would bundle oplines together.
Added tests for anonymous classes, callables and closures.
Added many more test cases for default expressions.
Added more test cases for default expressions.
Added test for default with variadic arguments.
This PR adds
default
as an expression in argument lists; it is not valid in any context other than argument lists. At runtime,default
will be substituted with the default value as declared at the call site of the function so called. For full details, see the default expression RFC.TODO
Limitations
The engine cannot retrieve the default value of a trampoline call.
Special thanks