Conversation
TimWolla
left a comment
There was a problem hiding this comment.
The refactoring for the zend_ast_fcc struct to hold parameters make sense to me to ship independently, but the grammar changes visible to userland (e.g. the support for ?) should probably be deferred to a later PR to not expose an incomplete implementation to users.
|
Right. It seemed reasonable to me for master, since I'm going to implement the rest soon. I've pushed a change to emit a compile error when trying to use |
Zend/zend_compile.c
Outdated
|
|
||
| zend_ast_list *args = zend_ast_get_list(((zend_ast_fcc*)args_ast)->args); | ||
| if (args->children != 1 || args->child[0]->attr != _IS_PLACEHOLDER_VARIADIC) { | ||
| zend_error_noreturn(E_COMPILE_ERROR, "Cannot create a Closure for call expression with more than one arguments, or non-variadic placeholders"); |
There was a problem hiding this comment.
The "Cannot create a Closure" part is copied from "Cannot create Closure for new expression" above
TimWolla
left a comment
There was a problem hiding this comment.
Didn't see any major issues as far as I'm qualified to tell.
Zend/zend_types.h
Outdated
| /* used for PFAs/FCCs */ | ||
| #define _IS_PLACEHOLDER_ARG 20 | ||
| #define _IS_PLACEHOLDER_VARIADIC 21 |
There was a problem hiding this comment.
Are these required to be a zval type for the following implementation? Other AST attributes use a “distinct” set of values.
There was a problem hiding this comment.
Good point. I'm now using a separate constant for AST attributes, and I was able to keep only one of these 2.
| if (list->kind == ZEND_AST_CALLABLE_CONVERT) { | ||
| zend_ast_fcc *fcc_ast = (zend_ast_fcc*)list; | ||
| fcc_ast->args = zend_ast_list_add(fcc_ast->args, arg); | ||
| return (zend_ast*)fcc_ast; |
There was a problem hiding this comment.
Not sure if this is clearer (the fcc_ast doesn't actually change):
| return (zend_ast*)fcc_ast; | |
| return list; |
There was a problem hiding this comment.
I prefer return (zend_ast*)fcc_ast, but no strong opinion
TimWolla
left a comment
There was a problem hiding this comment.
I am no longer seeing an issue, but please also seek another approval from someone more familiar with engine code.
ndossche
left a comment
There was a problem hiding this comment.
Only took a brief look but I don't see an obvious problem
|
|
||
| argument_list: | ||
| '(' ')' { $$ = zend_ast_create_list(0, ZEND_AST_ARG_LIST); } | ||
| '(' ')' { $$ = zend_ast_create_arg_list(0, ZEND_AST_ARG_LIST); } |
There was a problem hiding this comment.
zend_ast_create_arg_list is always used with ZEND_AST_ARG_LIST. Looks a bit redundant but perhaps this is for the future proofing.
There was a problem hiding this comment.
I considered this, but the variadic specialization trick implicitly expects the kind argument before the variadic ones, and I didn't want to create another variant of the specialization macro.
zend_ast_create_arg_list(init_children, ...) is defined as ZEND_AST_SPEC_CALL(zend_ast_create_arg_list, __VA_ARGS__), so that a call to zend_ast_create_arg_list() expands to zend_ast_create_arg_list_N(__VA_ARGS__) with N the number of va_args minus 1. As a result, zend_ast_create_arg_list(0, child1) expands to zend_ast_create_arg_list_0(child1), which is incorrect (should be zend_ast_create_arg_list_1(child1)).
|
Heads up: This broke this build: https://github.com/php/php-src/actions/runs/20767748729/job/59637430230 Might be expected given this is only a partial implementation. |
This is unexpected. This PR was designed to be a self-contained refactoring that should not break anything. See also my initial comment in #20717 (review). |
Spinned off arnaud-lb#22
RFC: https://wiki.php.net/rfc/partial_function_application_v2
For FCCs, the parser generates a normal function call AST node, the but argument list is a
ZEND_AST_CALLABLE_CONVERT/zend_ast_fccnode.We extend this for PFAs so that
zend_ast_fcccan represent arguments.In this PR:
zend_ast_fccso that arguments can be representedzend_ast_fccarguments in SHM / file cachezend_ast_arg_list_add(): Same aszend_ast_list_add(), but wraps the list in aZEND_AST_CALLABLE_CONVERTwhen adding any placeholder argument.Technically the arg list wrapping is not required, but it results in simpler code later as it will be very convenient in the compiler (determines whether a function calls is a PFA/FCC), and for PFA-in-const-expr support. It also allows to unify FCCs and PFAs in the grammar.