RFC: Expectations

OK thanks.
I'm switching to another task :)

Thanks. Dmitry.

Updated RFC as requested.

I'm just going to wait to hear what Nikita says, especially about
optimizing away unqualified calls to assert.
I guess this could be viewed as a BC break, it seems like quite a nice
break but not sure.

Will open voting when we hear from Nikita.

Cheers
Joe

On Wed, Feb 18, 2015 at 6:23 AM, Joe Watkins pthreads@pthreads.org
wrote:

Will update RFC today, thanks for working on it Dmitry :)

Cheers
Joe

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.

Thanks. Dmitry.

On Tue, Feb 17, 2015 at 5:11 PM, Nikita Popov nikita.ppv@gmail.com
wrote:

On Mon, Feb 16, 2015 at 4:47 PM, Dmitry Stogov dmitry@zend.com
wrote:

Hi Nikita,

it looks like a part of old implementation is not trivial with new
AST compiler.

previously we translated assert(condition) into assert(condition,
"assert(condition)").
actually we just captured a part of input buffer and added missing
string argument.

Is there a simple way to do the same now?

Not very simple, but we could store pointers/offsets to the start and
end of the fcall in the ast node, similar to what is done with lex_pos for
declarations: http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_ast.h#177

It would be nice if we could have precise offset information for all
nodes (this is particularly valuable if the ast is exposed to userland),
but that would increase memory usage during compilation, not sure if it's
worthwhile.

It's possible to convert AST into string using recursive
pretty-printer.
It's not a simple task itself, but it may be reused for other things.

I went by this way... :)
It must be ready soo.

Joe, the rest (including zero-cost assert) is implemented at
https://github.com/php/php-src/pull/1088/files
9 related tests are failed for now.

As this is implemented right now, it would require writing \assert()
in order to be zero-cost. Maybe we should disallow redefinition of assert
as a namespaced function, so we can always optimize this?

good catch.

Thanks.

Nikita

Morning,

I hear the concern regarding custom exceptions. Things will be used badly
whatever. It's easy to imagine that in a simple application you just don't
need to specify custom exceptions. But in a large codebase, being able to
structure exceptions is invaluable, it gives documentation reference points
and makes stack traces easier to read at a glance, because they are more
meaningful.

So I think we should keep the ability to throw custom exceptions.

I agree about making the exception part of a new tree, so that they are not
caught, so I'll just wait for that from dmitry and open the vote.

Cheers
Joe

On Wed, Feb 18, 2015 at 5:44 PM, Nikita Popov nikita.ppv@gmail.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