Use fast text conversion filters to implement mb_convert_variables #9966
Conversation
| } | ||
| zend_string *ret = php_mb_convert_encoding_ex(Z_STRVAL_P(var), Z_STRLEN_P(var), to_encoding, from_encoding); | ||
| zval_ptr_dtor(orig_var); | ||
| ZVAL_STR(orig_var, ret); |
There was a problem hiding this comment.
This is where I would like to ask someone who knows PHP internals well... am I handling the refcounts correctly?
mb_convert_variables replaces strings in an array or object. After converting the old zend_string to the new one, I am using zval_ptr_dtor to drop the reference to the old one, then ZVAL_STR to make the zval point to the new string.
I don't think ZVAL_STR modifies the refcount, and when the new zend_string is returned from php_mb_convert_encoding_ex, it should start with a refcount of 1, so hopefully we are OK here... 😕
There was a problem hiding this comment.
This seems to make sense, but you can always run some tests with Valgrind or ASAN to see if there is any issue. But been staring at this for a bit and it seems to check out
There was a problem hiding this comment.
Ah, that's a good idea.
| // JIS string (BASE64 encoded) | ||
| $jis = base64_decode('GyRCRnxLXDhsJUYlLSU5JUgkRyQ5ISMbKEIwMTIzNBskQiM1IzYjNyM4IzkhIxsoQg=='); | ||
| // EUC-JP string | ||
| $euc_jp = '日本語テキストです。01234５６７８９。'; |
There was a problem hiding this comment.
Hmm, interesting to see that GitHub is able to recognize this EUC-JP encoded string and display it correctly here...
On the other side, I've converted it to UTF-8 so that it will (hopefully) appear correctly in most text editors.
There was a problem hiding this comment.
I agree that it is preferable to have *.phpt files encoded as UTF-8 whenever possible.
| // JIS string (BASE64 encoded) | ||
| $jis = base64_decode('GyRCRnxLXDhsJUYlLSU5JUgkRyQ5ISMbKEIwMTIzNBskQiM1IzYjNyM4IzkhIxsoQg=='); | ||
| // EUC-JP string | ||
| $euc_jp = '日本語テキストです。01234５６７８９。'; |
There was a problem hiding this comment.
I agree that it is preferable to have *.phpt files encoded as UTF-8 whenever possible.
|
This was already merged. |
3 participants
FYA @cmb69 @Girgias @nikic @kamil-tekiela