On Wed, Feb 18, 2015 at 5:44 PM, Nikita Popov <nikita....@gmail.com> wrote:
> On Tue, Feb 17, 2015 at 10:50 PM, Dmitry Stogov <dmi...@zend.com> wrote: > >> Hi Joe >> >> The patch is ready https://github.com/php/php-src/pull/1088/files >> >> 1) I implemented AST pretty-printer to reconstruct the source. It may be >> reused in Reflection and other places through >> >> ZEND_API zend_string *zend_ast_export(const char *prefix, zend_ast *ast, >> const char *suffix); >> >> 2) zend.assertions=-1 - makes zero-cost asserts >> >> 3) assert() in a namespace leads to call a function defined in this >> namespace (if it's defined), but zend.assertions is still may disable this >> call or even prevent code generation for it. it's possible to use \assert() >> to call the system function. >> >> Please, make update RFC, add notes about (2) and (3). >> Then, it should be ready for voting. >> >> Nikita, please take a quick look over the patch. I hope, you don't have >> objections. >> > > I've added a few comments on the PR. > Thank you very much. You found about 25 bugs :) All of them except for "elseif" should be fixed now. I also think printing "else if" instead of "elseif" is not a big problem. Pretty-printer may also add or remove brackets in some situations. > > Two general notes on the RFC: > > 1. I don't like the ability to specify a different exception as the second > param. Assertions are supposed to be used as sanity checks during > development, not to throw meaningful and specialized exception types. > Having this possibility will probably only encourage bad usage of assert(). > It's not a big problem to me, but I'd rather not have this "feature". > Joe, this is part of your old patch. I really don't care about it. > 2. Similar to the EngineExceptions RFC I'm wondering if AssertionException > should extend Exception or be in a separate hierarchy. The same argument as > with engine exceptions applies: It's pretty unlikely that you want to catch > an AssertionException anywhere apart from top-level error handling code and > that people using catch(Exception) blocks would accidentally catch > assertions. I'm not sure I agree with this, but I wanted to mentioned the > concern. > This may be changed together with EngineException patch. I started working on it, and I hope I'll show you results tomorrow. Thanks. Dmitry. > > Nikita > >
