Hi,
I’ve checked the patch and tested the build, both looks fine. Though I would go by the suggestion from Dmitry about the new inlined functions. At least VC produces same ASM, probably gcc would produce good results as well. However, to operate just on one struct member a function looks like too much. Also we test mostly gcc and vc, but macros would ensure it works properly with compilers we don’t test. Also probably it would be a bit simpler from the dev perspective. I wouldn’t insist on this, but imho macros would make more sense. But all in all, if Dmitry’s tests bring positive results, we’re likely to accept this patch for 7.0. Regards Anatol From: Dmitry Stogov [mailto:dmi...@zend.com] Sent: Friday, June 26, 2015 12:59 PM To: francois Cc: Anatol Belski; PHP Internals Subject: Re: Improved zend_string API Hi Francois, >From the source code review, I don't see any problems. May be too aggressive usage of inline functions in zend_string implementation, but I hope, it shouldn't make any harm. I'll need to check, if C compilers are smart enough to produce good optimized code after inlining. In general ZSTR_xxx API is more consistent to me. I would even add ZSTR_...() twins for all zend_string_...(), and recommend to use ZTR_xxx names. Then we may decide to remove zend_string_..., STR_... and other name inconsistencies (in the future). In any case, I'll be able to test the PR only on Monday. Anatol, I would suggest to accept this, like an additional more consistent API (keeping the old names). But if it makes any troubles, delays or significant risks for 7.0 release process, I would say no. Thanks. Dmitry. On Fri, Jun 26, 2015 at 4:05 AM, François Laupretre <franc...@php.net <mailto:franc...@php.net> > wrote: Hi Dmitry, I have created a separate PR (https://github.com/php/php-src/pull/1367) containing just the zend_string API enhancements we've been talking about. It integrates the new ZSTR_VAL/ZSTR_LEN/ZSTR_HASH, renaming of STR_xxx to ZSTR_, plus compatibility macros and a pair of utility functions. I have incorporated a commit which changes the macros names from STR_ to ZSTR_ everywhere they are used in the distrib. I hope you can incorporate this as part of phpng for 7.0. It would be important to provide an API so that people stop using ->val/->len directly as soon as possible. I give up on the 'api-checks' and 'strict-api' configure flags. ATM, I don't have the energy to go through an RFC process which has every chances to fail. Maybe later... Regards François
