Move a few custom type checks to ZPP #6034
Conversation
kocsismate
changed the title
kocsismate
force-pushed the
kocsismate:more-accurate-types2
branch
2 times, most recently
from
735e36d
to
ed8e8d8
kocsismate
force-pushed the
kocsismate:more-accurate-types2
branch
from
ed8e8d8
to
edede27
| zend_string *field_str; | ||
| zend_string *field_str, *pv_field_str; | ||
| zend_long pv_field_long; |
carusogabriel
Aug 22, 2020
•
Member
Quick question: moving from zvals to zend_(string|int|etc..) types, are they cheaper?
Also, is that a good practice to follow?
kocsismate
Sep 1, 2020
Author
Member
@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
kocsismate:more-accurate-types2
branch
3 times, most recently
from
d7e055e
to
fc60c36
kocsismate
force-pushed the
kocsismate:more-accurate-types2
branch
3 times, most recently
from
16d3169
to
22abceb
| } | ||
| ret = mysql_options(mysql->mysql, mysql_option, (char *) &mysql_value_long); | ||
| } else { | ||
| ret = 1; |
nikic
Aug 25, 2020
Member
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.
kocsismate
Aug 27, 2020
Author
Member
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.
nikic
Sep 1, 2020
Member
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
Sep 1, 2020
Author
Member
I reverted all these changes (from mysqli and xml both), but stayed with the string|int PHPDoc type hints.
kocsismate
force-pushed the
kocsismate:more-accurate-types2
branch
from
22abceb
to
0126355
kocsismate
force-pushed the
kocsismate:more-accurate-types2
branch
2 times, most recently
from
9408fe8
to
7a26727
|
|
||
| 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)); |
nikic
Sep 1, 2020
Member
Drive-by note: I think the MAIL_ASCIIZ_CHECK_MBSTRING calls above are corrupting the argument strings :(
| } | ||
| ret = mysql_options(mysql->mysql, mysql_option, (char *) &mysql_value_long); | ||
| } else { | ||
| ret = 1; |
nikic
Sep 1, 2020
Member
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
kocsismate:more-accurate-types2
branch
4 times, most recently
from
44a5c7a
to
05ba96f
kocsismate
force-pushed the
kocsismate:more-accurate-types2
branch
from
05ba96f
to
8b07b3e
No description provided.