Re: [PHP-DEV] Zend language parser improvements
I don't see technical problems with the patch. However, I also don't see any significant benefits. From the user perspective it'll just change error messages and prevent final final class declarations. Nikita, what do you think? Thanks. Dmitry. On Fri, Dec 5, 2014 at 8:18 PM, guilhermebla...@gmail.com guilhermebla...@gmail.com wrote: Hi guys, I'd really appreciate some review around the before-mentioned PRs. I have added a new one to the list now: - https://github.com/php/php-src/pull/937 This PR addresses the parsing support for traits to have extends and implements, as they are invalid. There's another one in the oven, which prevents extension developers to create classes that extends traits or interfaces. This is currently supported only for userland classes, but not for Zend API. Cheers, On Wed, Dec 3, 2014 at 8:39 PM, guilhermebla...@gmail.com guilhermebla...@gmail.com wrote: On Wed, Dec 3, 2014 at 8:06 PM, Levi Morrison le...@php.net wrote: The parser changes need to be careful reviewed; I don't have time at the moment to verify it but I think you unintentionally allowed some syntax's that shouldn't be valid because of the addition to `inner_statement`. Shouldn't. I broke down class_declaration_statement into 3 pieces: class_declaration_statement, interface_declaration_statement and trait_declaration_statement. At the end, all I've done is adding the other 2 new rules back to where it was consumed. Maybe I just looked too quickly. In any case, parser changes should always get several people reviewing them. Agreed. =) -- Guilherme Blanco MSN: guilhermebla...@hotmail.com GTalk: guilhermeblanco Toronto - ON/Canada -- Guilherme Blanco MSN: guilhermebla...@hotmail.com GTalk: guilhermeblanco Toronto - ON/Canada
Re: [PHP-DEV] Zend language parser improvements
On Thu, Dec 4, 2014 at 1:46 AM, guilhermebla...@gmail.com guilhermebla...@gmail.com wrote: Examples are COM, PDO Statement, DOM XML and MySQLi. They also reduce the amount of checks of bison when parsing a php file, it provides a nicer fatal error around multiple final and multiple abstract mix between abstract and final instead of a generic parser error. PDO statement should not be made final, or this is a breaking language change (requiring an RFC and a 2/3 majority). Drupal 7 does extend PDOStatement without any problem. Damien -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Zend language parser improvements
On Mon, Dec 8, 2014 at 1:30 PM, Damien Tournoud d...@damz.org wrote: PDO statement should not be made final, or this is a breaking language change (requiring an RFC and a 2/3 majority). I see from the PR that it's PDORow, not PDOStatement. That one is not actually exposed as such to userspace, so it is fine. Damien -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Zend language parser improvements
Hi Dmitry, As I said, these patches are not huge beneficial, but enablers. Enablers in the sense that further development over class/interface/trait simplified. It's not a short win benefit, but a mid/long term. Also defer currently checks done in zend_compile to Bison (such as trait extends and implements), addresses small inconsistencies (such as an extension class that extend an interface) and also easier to comprehend for newcomers (such as trait flag value). Now development that happens over this (such as abstract final/static class, package private class) become easy, as you can see in PR for static class for example. Cheers, On Mon, Dec 8, 2014 at 7:33 AM, Damien Tournoud d...@damz.org wrote: On Mon, Dec 8, 2014 at 1:30 PM, Damien Tournoud d...@damz.org wrote: PDO statement should not be made final, or this is a breaking language change (requiring an RFC and a 2/3 majority). I see from the PR that it's PDORow, not PDOStatement. That one is not actually exposed as such to userspace, so it is fine. Damien -- Guilherme Blanco MSN: guilhermebla...@hotmail.com GTalk: guilhermeblanco Toronto - ON/Canada
Re: [PHP-DEV] Zend language parser improvements
I'm not against the patch, I'm actually indifferent. Lets wait for Nikita's opinion. I took just a quick look over static class RFC. I'll need to think more about it. Thanks. Dmitry. On Mon, Dec 8, 2014 at 5:18 PM, guilhermebla...@gmail.com guilhermebla...@gmail.com wrote: Hi Dmitry, As I said, these patches are not huge beneficial, but enablers. Enablers in the sense that further development over class/interface/trait simplified. It's not a short win benefit, but a mid/long term. Also defer currently checks done in zend_compile to Bison (such as trait extends and implements), addresses small inconsistencies (such as an extension class that extend an interface) and also easier to comprehend for newcomers (such as trait flag value). Now development that happens over this (such as abstract final/static class, package private class) become easy, as you can see in PR for static class for example. Cheers, On Mon, Dec 8, 2014 at 7:33 AM, Damien Tournoud d...@damz.org wrote: On Mon, Dec 8, 2014 at 1:30 PM, Damien Tournoud d...@damz.org wrote: PDO statement should not be made final, or this is a breaking language change (requiring an RFC and a 2/3 majority). I see from the PR that it's PDORow, not PDOStatement. That one is not actually exposed as such to userspace, so it is fine. Damien -- Guilherme Blanco MSN: guilhermebla...@hotmail.com GTalk: guilhermeblanco Toronto - ON/Canada
Re: [PHP-DEV] Zend language parser improvements
On Mon, Dec 8, 2014 at 11:45 AM, Dmitry Stogov dmi...@zend.com wrote: I don't see technical problems with the patch. However, I also don't see any significant benefits. From the user perspective it'll just change error messages and prevent final final class declarations. Nikita, what do you think? Thanks. Dmitry. The three parts: 1. Use ZEND_ACC_FINAL instead of ZEND_ACC_FINAL_CLASS: Looks sensible, given how many extensions have confused this. We should be careful that this change does not make anything final that many people extended (even if it was originally meant to be final.) 2. Don't use magic value for ZEND_ACC_TRAIT: Also makes sense, the behavior of the current value is pretty unclear. 3. Changing the grammar to separate class/trait declarations and have a modifier list: This doesn't really make much sense as things currently are (only one modifier allowed). I'd suggest to change this when we actually support multiple modifiers (e.g. with the static classes patch). Nikita
Re: [PHP-DEV] Zend language parser improvements
Nikita, Shouldn't #3 make more sense taking into consideration this commit: https://github.com/guilhermeblanco/php-src/commit/872a97c8dbc1c8823985d9a0305938c508865a0d It is part of a followup PR https://github.com/php/php-src/pull/937 that removes compiler code checks and delegates to bison, since it's a grammar issue to accept trait Foo extends Bar. Regards, On Mon, Dec 8, 2014 at 3:06 PM, Nikita Popov nikita@gmail.com wrote: On Mon, Dec 8, 2014 at 11:45 AM, Dmitry Stogov dmi...@zend.com wrote: I don't see technical problems with the patch. However, I also don't see any significant benefits. From the user perspective it'll just change error messages and prevent final final class declarations. Nikita, what do you think? Thanks. Dmitry. The three parts: 1. Use ZEND_ACC_FINAL instead of ZEND_ACC_FINAL_CLASS: Looks sensible, given how many extensions have confused this. We should be careful that this change does not make anything final that many people extended (even if it was originally meant to be final.) 2. Don't use magic value for ZEND_ACC_TRAIT: Also makes sense, the behavior of the current value is pretty unclear. 3. Changing the grammar to separate class/trait declarations and have a modifier list: This doesn't really make much sense as things currently are (only one modifier allowed). I'd suggest to change this when we actually support multiple modifiers (e.g. with the static classes patch). Nikita -- Guilherme Blanco MSN: guilhermebla...@hotmail.com GTalk: guilhermeblanco Toronto - ON/Canada
Re: [PHP-DEV] Zend language parser improvements
Hi guys, I'd really appreciate some review around the before-mentioned PRs. I have added a new one to the list now: - https://github.com/php/php-src/pull/937 This PR addresses the parsing support for traits to have extends and implements, as they are invalid. There's another one in the oven, which prevents extension developers to create classes that extends traits or interfaces. This is currently supported only for userland classes, but not for Zend API. Cheers, On Wed, Dec 3, 2014 at 8:39 PM, guilhermebla...@gmail.com guilhermebla...@gmail.com wrote: On Wed, Dec 3, 2014 at 8:06 PM, Levi Morrison le...@php.net wrote: The parser changes need to be careful reviewed; I don't have time at the moment to verify it but I think you unintentionally allowed some syntax's that shouldn't be valid because of the addition to `inner_statement`. Shouldn't. I broke down class_declaration_statement into 3 pieces: class_declaration_statement, interface_declaration_statement and trait_declaration_statement. At the end, all I've done is adding the other 2 new rules back to where it was consumed. Maybe I just looked too quickly. In any case, parser changes should always get several people reviewing them. Agreed. =) -- Guilherme Blanco MSN: guilhermebla...@hotmail.com GTalk: guilhermeblanco Toronto - ON/Canada -- Guilherme Blanco MSN: guilhermebla...@hotmail.com GTalk: guilhermeblanco Toronto - ON/Canada
[PHP-DEV] Zend language parser improvements
Hi, I made some improvements that would like to get merged into PHP, but I require someone with Zend karma to review and merge. The patches are all related and included in each other as a progressive effort. As a quick explanation, the removal of ZEND_ACC_FINAL_CLASS addresses some classes that were not properly assigned as final. They're now final. Examples are COM, PDO Statement, DOM XML and MySQLi. They also reduce the amount of checks of bison when parsing a php file, it provides a nicer fatal error around multiple final and multiple abstract mix between abstract and final instead of a generic parser error. Finally, ZEND_ACC_TRAIT got updated to a saner value. It was pretty complex to comprehend why it was 0x120. It was done because a trait was considered internally as abstract because of instantiation and inheritance checking. This all got addressed too. Here they are: - ZEND_ACC_TRAIT value update: https://github.com/php/php-src/pull/931 (includes the next one) - Zend language parser class decoupling: https://github.com/php/php-src/pull/928 (includes the next one) - ZEND_ACC_FINAL_CLASS removal: https://github.com/php/php-src/pull/911 Theoretically, merging the first one would mean merging all, but I left them separate for proper case by case evaluation. Regards, -- Guilherme Blanco MSN: guilhermebla...@hotmail.com GTalk: guilhermeblanco Toronto - ON/Canada
Re: [PHP-DEV] Zend language parser improvements
The parser changes need to be careful reviewed; I don't have time at the moment to verify it but I think you unintentionally allowed some syntax's that shouldn't be valid because of the addition to `inner_statement`. Maybe I just looked too quickly. In any case, parser changes should always get several people reviewing them. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Zend language parser improvements
On Wed, Dec 3, 2014 at 8:06 PM, Levi Morrison le...@php.net wrote: The parser changes need to be careful reviewed; I don't have time at the moment to verify it but I think you unintentionally allowed some syntax's that shouldn't be valid because of the addition to `inner_statement`. Shouldn't. I broke down class_declaration_statement into 3 pieces: class_declaration_statement, interface_declaration_statement and trait_declaration_statement. At the end, all I've done is adding the other 2 new rules back to where it was consumed. Maybe I just looked too quickly. In any case, parser changes should always get several people reviewing them. Agreed. =) -- Guilherme Blanco MSN: guilhermebla...@hotmail.com GTalk: guilhermeblanco Toronto - ON/Canada
