Committed except of ZVAL_STR_INC/DEC_LEN() and some other unused macros. Thanks. Dmitry.
On Mon, Jun 29, 2015 at 4:23 PM, François Laupretre <[email protected]> wrote: > 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:[email protected]] > *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:[email protected] <[email protected]>] > *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 <[email protected]> > 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 > > >
