Move a few custom type checks to ZPP #6034
Conversation
kocsismate
changed the title
kocsismate
force-pushed
the
more-accurate-types2
branch
2 times, most recently
from
735e36d to
ed8e8d8
Compare
kocsismate
force-pushed
the
more-accurate-types2
branch
from
ed8e8d8 to
edede27
Compare
| zend_string *field_str; | ||
| zend_string *field_str, *pv_field_str; | ||
| zend_long pv_field_long; |
There was a problem hiding this comment.
Quick question: moving from zvals to zend_(string|int|etc..) types, are they cheaper?
Also, is that a good practice to follow?
There was a problem hiding this comment.
@carusogabriel I don't think they are noticably cheaper. The real benefit is being able to accept arguments whose type is validated according to the standard semantics, rather than in a custom way.
kocsismate
force-pushed
the
more-accurate-types2
branch
3 times, most recently
from
d7e055e to
fc60c36
Compare
kocsismate
force-pushed
the
more-accurate-types2
branch
3 times, most recently
from
16d3169 to
22abceb
Compare
ext/mysqli/mysqli_api.c
Outdated
| } | ||
| ret = mysql_options(mysql->mysql, mysql_option, (char *) &mysql_value_long); | ||
| } else { | ||
| ret = 1; |
There was a problem hiding this comment.
I'm a bit unsure about this one. If we consider a call like mysqli_options($link, MYSQLI_OPT_CONNECT_TIMEOUT, '100'), this was previously valid and the '100' was interpreted as 100. Now it will throw instead. That is, it's now necessary to match the type of the option, rather than the option forcing the type of the value.
There was a problem hiding this comment.
Yeah, it's a valid critique against the (former) implementation. Now, I've just pushed a change which tries to address this by trying to convert the type to the expected one if necessary. I like this solution, but I'm curious whether you see any problem with this approach.
There was a problem hiding this comment.
Not a fan of this. This has made the implementation more complicated than it was before.
I think it is better to keep this as a mixed argument. The fact that only int|string is accepted is rather incidential imho (e.g. there's no reason why in the future an option using float or bool value couldn't be added).
There was a problem hiding this comment.
I reverted all these changes (from mysqli and xml both), but stayed with the string|int PHPDoc type hints.
kocsismate
force-pushed
the
more-accurate-types2
branch
from
22abceb to
0126355
Compare
kocsismate
force-pushed
the
more-accurate-types2
branch
2 times, most recently
from
9408fe8 to
7a26727
Compare
|
|
||
| if (str_headers) { | ||
| tmp_headers = zend_string_init(ZSTR_VAL(str_headers), ZSTR_LEN(str_headers), 0); | ||
| MAIL_ASCIIZ_CHECK_MBSTRING(ZSTR_VAL(tmp_headers), ZSTR_LEN(tmp_headers)); |
There was a problem hiding this comment.
Drive-by note: I think the MAIL_ASCIIZ_CHECK_MBSTRING calls above are corrupting the argument strings :(
ext/mysqli/mysqli_api.c
Outdated
| } | ||
| ret = mysql_options(mysql->mysql, mysql_option, (char *) &mysql_value_long); | ||
| } else { | ||
| ret = 1; |
There was a problem hiding this comment.
Not a fan of this. This has made the implementation more complicated than it was before.
I think it is better to keep this as a mixed argument. The fact that only int|string is accepted is rather incidential imho (e.g. there's no reason why in the future an option using float or bool value couldn't be added).
kocsismate
force-pushed
the
more-accurate-types2
branch
4 times, most recently
from
44a5c7a to
05ba96f
Compare
kocsismate
force-pushed
the
more-accurate-types2
branch
from
05ba96f to
8b07b3e
Compare
No description provided.