follow-up: pdo_sqlite: Use the new FCC API#14059
follow-up: pdo_sqlite: Use the new FCC API#14059SakiTakamachi wants to merge 3 commits intophp:masterfrom
Conversation
SakiTakamachi
force-pushed
the
pdo_sqlite_zpp_no_trampoline
branch
3 times, most recently
from
4b27acf to
8091fba
Compare
SakiTakamachi
force-pushed
the
pdo_sqlite_zpp_no_trampoline
branch
from
8091fba to
f9b0c64
Compare
SakiTakamachi
changed the title
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
I mainly would like to se additional trampoline tests, as I don't even know if PDO has them
| ZEND_PARSE_PARAMETERS_START(2, 4) | ||
| Z_PARAM_STRING(func_name, func_name_len) | ||
| Z_PARAM_FUNC(fci, fcc) | ||
| Z_PARAM_FUNC_NO_TRAMPOLINE_FREE(fci, fcc) |
There was a problem hiding this comment.
This would leak if you have a trampoline and one of the follow-up arguments fails the ZPP check I think
There was a problem hiding this comment.
Oh, indeed. This will leak.
| zend_type_error("%s(): Return value of the callback must be of type int, %s returned", | ||
| ZSTR_VAL(func_name), zend_zval_value_name(&retval)); | ||
| zend_string_release(func_name); | ||
| return FAILURE; |
There was a problem hiding this comment.
While you're at it, can you change this return FAILURE, as I would like them to limit their use to functions that actually have a return type of zend_result.
Or this can be a follow-up PR.
There was a problem hiding this comment.
I will also follow up on this.
| zval retval; | ||
| int i; | ||
| int ret; | ||
| int ret = SUCCESS; |
There was a problem hiding this comment.
Either this function should return a zend_result or use integers.
It also looks like we don't even use the return value when calling this function.
There was a problem hiding this comment.
Agree. However, there are probably some other similar ones, so I will respond with a follow-up PR.
| struct pdo_sqlite_func *next; | ||
|
|
||
| zval func, step, fini; | ||
| int argc; |
There was a problem hiding this comment.
Aside: is this argc field even used?
There was a problem hiding this comment.
Yes, here is.
php-src/ext/pdo_sqlite/sqlite_driver.c
Line 108 in 6303d1f
2 participants
followup #9840, #9877 and #14040