Hi,
OK, I didn’t understand Dmitry’s comment on ‘aggressively’ using inline functions but, thanks to your comment, I understand now. It has nothing to do with performance. Using a function for ->val is useless, I agree, as it is read-only be nature. But using 2 functions for len provides read-only/write-only methods, which is not possible with a macro. As this is a new API, I thought it was the occasion to provide a high encapsulation level from the start. This will always be less work for the future. Dmitry suggested we rename everything using a ‘ZSTR_’ terminology, promote these names, and declare old names as compatibility macros. I’m currently working on this and will probably deliver it tonight. It will be a separate optional commit. You’ll see if time allows to include it. Anyway, this mustn’t be blocking inclusion in 7.0. Regards François De : Anatol Belski [mailto:anatol....@belski.net] Envoyé : vendredi 26 juin 2015 21:16 À : 'Dmitry Stogov'; 'francois' Cc : 'Anatol Belski'; 'PHP Internals' Objet : RE: Improved zend_string API 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> 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
