On Mon, Aug 18, 2014 at 1:19 PM, Dmitry Stogov <dmi...@zend.com> wrote:
> On Mon, Aug 18, 2014 at 2:26 PM, Nikita Popov <nikita....@gmail.com>
> wrote:
>
>> On Mon, Aug 18, 2014 at 9:32 AM, Dmitry Stogov <dmi...@zend.com> wrote:
>>
>>> Hi Nikita,
>>>
>>> I think RFC misses few important notes about behavior changes:
>>>
>>> 1) The behavior of $a->$b[$c] is changed. PHP apps that used such syntax
>>> have to be changed into $a->{$b[$c]}.
>>>
>>
>> This is a change from the uniform variable syntax (
>> https://wiki.php.net/rfc/uniform_variable_syntax), not due to the AST.
>> The AST patch already includes the uniform variable syntax patch for
>> technical reasons.
>>
>
> I missed it. I think you should add a clear note that AST RFC also
> implements "Uniform Variable Syntax" (completely or with some exceptions).
>
>
>>
>>
>>> 2) The evaluation order of left and right parts of assignment is
>>> changed. $a[$i++] = $a[$i++]; It wasn't guaranteed before, but it may break
>>> some code anyway.
>>>
>>
>> This is no longer true, the AST now implements left-to-right evaluation
>> for assignments.
>>
>
> great. I don't talk that the old evaluation order better or worse. I just
> don't see a big reason to change it.
>
>
>>
>>
>>> 3) $a=[1,2]; list($a,$b) = $a; won't work in the same way as before
>>>
>>
>> Yes, this is true. I think assigning list() elements from left-to-right
>> makes a lot more sense and I also think that this is the kind of behavior
>> that it's okay to change in a major version. In any case, I've added this
>> particular example to the list() section of the RFC as well.
>>
>
> thanks.
>
>
>>
>>
>>> 4) Usage of undefined constants now leads to fatal errors? (see
>>> Zend/tests/use_const/no_global_fallback.php)
>>>
>>
>> This was a bug in the previous implementation - it uses a namespaced
>> constant "foo\bar\baz" and usage of undefined namespaced constants results
>> in a fatal error. This does not change that undefined unqualified constants
>> (like just "baz") will results in a notice and string fallback. This only
>> fixes the resolution of "use const" imports.
>>
>> 5) The patch also enables expressions on constant arrays e.g.
>>> isset(arra(1,2,3)[$x]);
>>>
>>
>> This is also a change from the uniform variable syntax, not from the AST.
>>
>>
>>> Personally, I would prefer to separate syntax and behavior changes
>>> from the AST patch itself or at least don't miss such changes, because they
>>> must be very significant from users point of view.
>>>
>>
>> I think the confusion here is mainly that the AST patch already includes
>> the UVS patch (which was already voted in previously). I didn't list the
>> changes from the UVS patch in this RFC, because they're already listed in
>> https://wiki.php.net/rfc/uniform_variable_syntax.
>>
>>
>>> Also some implementation related notes:
>>>
>>> - Changes to Zend/tests/bug43450.phpt are unrelated to AST. It's just a
>>> bad test that may be fixed separately.
>>>
>>
>> The AST makes it fail due to different memory usage patterns, which is
>> why I changed it to use a test compatible with different peak memory usage.
>>
>
> This test is ugly, and it starts fail from time to time after changes in
> absolutely unrelated areas :(
>
Maybe we should just remove the memory usage checks and rely on ZMM
warnings in debug mode?
>
>
>>
>> - I didn't get why you deleted Zend/tests/errmsg_014.php
>>>
>>
>> I allowed doing $obj->__clone() calls, because __clone was the only magic
>> method that had a compile-time checks preventing some calls to it. If we
>> allow all other magic methods to be called, then there's no reason to
>> forbid __clone. I've added a note about this to the RFC.
>>
>
> ok. I can't remember why it prohibited.
>
>
>>
>>
>>> - ZEND_INIT_FCALL_BY_NAME handler shouldn't check if constant operand
>>> is a string. Compiler must not provide non-string constant operand. (All
>>> the changes to ZEND_INIT_FCALL_BY_NAME need to be verified more careful)
>>>
>>
>> How should this be implemented instead? The case where an IS_CONST
>> operand is called needs to be handled somewhere. One possible approach
>> would be to send all constant-string calls through ZEND_INIT_FCALL. It uses
>> the same code code as ZEND_INIT_FCALL_BY_NAME with the difference that
>> BY_NAME has an extra zval literal storing the original (non-lowercased)
>> function name. We could store this additional function name only in cases
>> of a non-ct-bound function, because that's the only case where the
>> undefined function error can be thrown. Does that make sense?
>>
>
> I mean the additional check (Z_TYPE_P(opline->op2.zv) == IS_STRING) at
> line 2391 must be useless.
> Am I wrong?
>
> or is it intended to handle ["class_name", "method_name"]() syntax?
>
Yes, that's the case it handles.
>
>>
>>
>>
>>> - The same in ext/opcache/Optimizer/optimize_func_calls.c, but it's not
>>> critical here.
>>>
>>> - I think you don't need IS_CONST operand for ZEND_ISSET_ISEMPTY_PROP_OBJ
>>> handler.
>>>
>>> - Can OPcahce always keep AST in shared memory and don't copy it into
>>> process memory on each request? (
>>> ext/opcache/zend_accelerator_util_funcs.c)
>>>
>>
>> I'm not particularly familiar with opcache, so I'm not sure whether this
>> is possible. Right now this doesn't work because the constant expression
>> AST is heap allocated, so we have efree calls. But if we allocate the
>> constant-expression AST in CG(arena), maybe this would be possible?
>>
>
> If the constant expression AST is not changed at run-time it may be kept
> in SHM.
> In this case you may just avoid copying from SHM to process memory and
> keep pointers to AST in SHM.
> Probably, some special flags should be introduced to avoid such AST
> destruction.
>
> You may run PHP disable the AST copying code and run PHP with
> "opcache.protect_memory=1".
> Once PHP will try to modify the SHM contents, it'll crash.
>
Looking at the AST evaluation code, I think this would be problematic:
http://lxr.php.net/xref/phpng/Zend/zend_ast.c#286 Here the constant is
updated in-place.
Nikita
