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. 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". 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. Nikita
