[PHP-DEV] RE: Improved zend_string API
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
[PHP-DEV] Re: Improved zend_string API
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
[PHP-DEV] RE: Improved zend_string API
Hi Dmitry, I just commited some additional changes, following your suggestions : - Renamed every ‘zend_string_xxx()’ functions to ‘ZSTR_’-prefixed uppercase names, - Defined compatibility macros for old names - Changed the rest of the code to use new ZSTR_ names - A special case for zend_string_tolower(), which I found in ‘zend_operators.c’. I renamed it but didn’t move it to zend_string.c/.h. You probably know better than me if there’s a good reason to define it there or if it should be moved. I also made ZSTR_LEN() read-only (ZSTR_SET_LEN() must be used for write operations), making the zend_string API ‘clean’ in terms of separate getters/setters (Z_STRLEN() remains a read-write macro, no BC break here). I removed the functions you removed. Anyway, increment/decrement functions will need to be defined when we migrate the code to use the API only because the code often increments/decrements string length. As ZSTR_LEN() is read-only, we cannot use ‘++’ with it, and providing increment/decrement functions at the zend_string level is the only way to avoid writing ‘ZSTR_SET_LEN(s,ZSTR_LEN(s) + 1)’. I hope we (and especially you) still have enough time to integrate this, especially the new ‘ZSTR’ naming scheme. Anyway, I’m glad you could commit the most important additions for 7.0. Regards François -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] RE: Improved zend_string API
Hi, De : François Laupretre [mailto:franc...@php.net] Envoyé : mardi 30 juin 2015 03:08 À : 'Dmitry Stogov' Cc : 'Anatol Belski'; 'PHP Internals' Objet : [PHP-DEV] RE: Improved zend_string API Hi Dmitry, I just commited some additional changes, following your suggestions : - Renamed every ‘zend_string_xxx()’ functions to ‘ZSTR_’-prefixed uppercase names, - Defined compatibility macros for old names - Changed the rest of the code to use new ZSTR_ names - A special case for zend_string_tolower(), which I found in ‘zend_operators.c’. I renamed it but didn’t move it to zend_string.c/.h. You probably know better than me if there’s a good reason to define it there or if it should be moved. I also made ZSTR_LEN() read-only (ZSTR_SET_LEN() must be used for write operations), making the zend_string API ‘clean’ in terms of separate getters/setters (Z_STRLEN() remains a read-write macro, no BC break here). I removed the functions you removed. Anyway, increment/decrement functions will need to be defined when we migrate the code to use the API only because the code often increments/decrements string length. As ZSTR_LEN() is read-only, we cannot use ‘++’ with it, and providing increment/decrement functions at the zend_string level is the only way to avoid writing ‘ZSTR_SET_LEN(s,ZSTR_LEN(s) + 1)’. I hope we (and especially you) still have enough time to integrate this, especially the new ‘ZSTR’ naming scheme. Anyway, I’m glad you could commit the most important additions for 7.0. I hadn't noticed that the PR had been closed. The changes described in my previous message are now at : https://github.com/php/php-src/pull/1381 Regards François -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] 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 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
[PHP-DEV] 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
