> Am 29.06.2015 um 15:46 schrieb Dmitry Stogov <dmi...@zend.com>: > > 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 <franc...@php.net> > 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: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 <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
Hey, looks like I'm a bit late to the party, but I'd like to object against the accessor macros on zend_string. What does it help to replace very nice ->val, ->len etc. with macros? ZSTR_VAL(str)[ZSTR_LEN(str)] = 0; or str->val[str->len] = 0; zend_string_set_len(str, ZSTR_LEN(str) - 2); or str->len -= 2; Also, that particular function suggests to me that the string is expanded if lengths don't match. I much prefer direct accessors which indicate a) no side-effects happening here, b) less functions to remember (you just look at the struct and know everything about it) and c) end up being more readable. Yes, I'm totally aware that when we ever want to change the API (I don't see coming it in near future anyway), it could help having wrappers. But wrappers don't help against every API change… let's say for some reason we'd want to go back to the old char*/int; would wrappers help? no. (Yes, that's maybe a bad example, but just trying to show that, while it has maybe a theoretical gain, I don't see a real practical benefit.) zend_string is really a core structure and extremely simple. Please don't trade simplicity for a small other benefit. Also, we still leave zend_string struct exposed (presumably for allowing inlining). At the point where we are now, many extension authors probably already are also accessing zend_string directly. We're going to have a colorful mix of code using the APIs and not using it, which is the worst we can get now. To end up, I very much like unifying prefixes (ZSTR_*), but I strongly dislike hurting readability and consistency so much for such little benefit. Thanks, Bob -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
