Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:72059
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain pthreads.org from 74.125.82.170 cause and error)
Message-ID: <52EF5CE1.8060903@pthreads.org>
Date: Mon, 03 Feb 2014 09:09:53 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7
MIME-Version: 1.0
To: Stas Malyshev <smalyshev@sugarcrm.com>, 
 PHP internals <internals@lists.php.net>,
 Dmitry Stogov <dmitry@zend.com>
References: <52EE2B1D.7000307@pthreads.org> <52EEA7BE.4050500@sugarcrm.com> <52EEF636.7010505@pthreads.org> <52EF5588.6010902@sugarcrm.com>
In-Reply-To: <52EF5588.6010902@sugarcrm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] RFC: expectations/assertions
From: pthreads@pthreads.org (Joe Watkins)

On 02/03/2014 08:38 AM, Stas Malyshev wrote:
> Hi!
>
>> Well lets not forget that we already have an implementation as part of
>> the core, so to argue that we don't require assert in the core is moot;
>> we already have it.
> I'm not sure what you mean. We have assert function, a function among
> others. What you propose is special engine-level magic just for this
> function. The difference is like having a program that runs on Linux and
> does X and having special patch in Linux kernel just to do X. The bar
> for the latter is much higher.
>
>> While you can avoid some overhead in userland, you cannot remove the
>> overhead completely; a good assertion API should have no impact on
>> production, none, it should require no boilerplate code to make sane use
>> of it, at all, since it is meant to be a core feature.
> This is not completely fulfilled by your patch either - it still has an
> overhead of having the opcodes there and doing the jumps. If the code
> you're avoiding to run is heavy, then of course this overhead is
> negligible compared to the code you're avoiding, but the same then is
> true for purely userspace solution.
>
>> It would seem to create inconsistency, but only because it's compatible
>> with legacy code asserting strings, if you are smart, you will not
> It doesn't "seem" to create inconsistency. It creates it. It's not like
> I'm imagining two parameters doing the same thing :)
>
>> assert with strings and take advantage of the new implementation, if you
>> are not, you will continue to assert strings and be unaffected by the
>> new implementation ... that seems ideal to me, and Dmitry ...
> I don't think having a function that is half a function and half a magic
> engine constraint and that is controlled by two separate settings, one
> living in zend. and one in assert. is really ideal. Also, it is not true
> that string parameter is the only thing that controls it - as I already
> showed, how you call it also makes it work completely different and
> assert.* parameters still influence it even in magic mode, leading to
> such things as both throwing exceptions and bailing out - which I still
> don't understand how it's supposed to work.
>

> This is not completely fulfilled by your patch either - it still has an
> overhead of having the opcodes there and doing the jumps. If the code
> you're avoiding to run is heavy, then of course this overhead is
> negligible compared to the code you're avoiding, but the same then is
> true for purely userspace solution.

As it says in the RFC, the complete removal will be implemented before 
merge, the implementation allows the complete removal, the one in the 
core doesn't.
The original patch didn't have this limitation but wasn't compatible 
with the existing assert API, so dmitry decided this way was better.

> It doesn't "seem" to create inconsistency. It creates it. It's not like
> I'm imagining two parameters doing the same thing :)

It doesn't create inconsistency in code currently using assert, and 
there's no such thing as inconsistent code that doesn't exist, so I 
don't see the inconsistency you think is definitely there, I see how you 
think it's there, without really considering it, but it is not in fact 
there. Code currently using assert will behave in the same way, code 
taking advantage of exceptions and expressions as arguments will behave 
in the way the patch dictates. I see no inconsistency.

> I don't think having a function that is half a function and half a magic
> engine constraint and that is controlled by two separate settings, one
> living in zend. and one in assert. is really ideal. Also, it is not true
> that string parameter is the only thing that controls it - as I already
> showed, how you call it also makes it work completely different and
> assert.* parameters still influence it even in magic mode, leading to
> such things as both throwing exceptions and bailing out - which I still
> don't understand how it's supposed to work.

Nor do I, the original patch wasn't this, it was a new syntax with this 
API that solved the problems this patch is solving in the same way but 
by superseding it's functionality rather than trying to retain 
compatibility with something that is crappy. Since that turned out to 
not be an option - we have to retain compatibility with the current 
assert api - the current patch is the only way of moving forward, even 
if you can see ways of making it do strange things (as a result of 
retaining compatibility), the perfectly clean patch, with no strangeness 
whatsoever, was rejected because we don't like to introduce new keywords 
or syntax or blah blah blah ...

If you use the configuration you had before it will behave as it did 
before, if you setup the new configuration and use the new api it will 
behave as the patch describes, that you can purposefully configure or 
otherwise force php to do strange things is nothing new.

I'm going to move it to vote today, there's not much point in talking 
about it, the discussions have already been had and the patch rewritten 
to satisfy the complaints of most people, and has also been available 
for many many months, so if there is a suitable patch, this is it, I 
don't want to waste more time on it if we're not going to move forward.

Cheers
Joe