Separate static property assignment out into dedicated assign ops #2226
Conversation
|
Comment on behalf of krakjoe at php.net: labelling |
Zend/zend_execute.c
Outdated
| zend_string_release(name); | ||
| } | ||
| return NULL; | ||
| } |
Zend/zend_execute.c
Outdated
| zend_throw_error(NULL, "Access to undeclared static property: %s::$%s", ZSTR_VAL(ce->name), ZSTR_VAL(name)); | ||
| if (varname_op_type != IS_CONST) { | ||
| zend_string_release(name); | ||
| } |
There was a problem hiding this comment.
This check looks redundant. polymorphic_cache_slot requires op1 to be const (this branch is also missing in the same code below)
There was a problem hiding this comment.
Uh, does it actually require it being const, or does it just happen that it's only used for when var_op_name is const? If it's the latter, that check anyway doesn't harm as it will be compiled out when inlining?
There was a problem hiding this comment.
It requires it because cache slots are bound to CONST operands. In any case, I don't really care either way, as long as the two code blocks are the same.
There was a problem hiding this comment.
It's missing there because the condition of the whole block there already includes varname_op_type == IS_CONST
| #if !defined(ZEND_VM_SPEC) || (OP2_TYPE != IS_UNUSED) | ||
| ZEND_VM_DISPATCH_TO_HELPER(zend_binary_assign_op_static_prop_helper, binary_op, binary_op); | ||
| #else | ||
| # if !defined(ZEND_VM_SPEC) || OP1_TYPE != IS_UNUSED |
There was a problem hiding this comment.
Wouldn't it be better to formulate all these conditions positively? (I.e., the same as the two above)
There was a problem hiding this comment.
Dunno, that was already that way originally… And not sure whether it's better listing every single of the four individually?
There was a problem hiding this comment.
Yeah, leave it. I just found it weird that part is implemented one way and part the other way.
bwoebi
force-pushed
the
extra_static_prop_assign_ops
branch
2 times, most recently
from
6c66571 to
0bf02dc
Compare
| } | ||
|
|
||
| ZEND_VM_INLINE_HELPER(zend_binary_assign_op_helper, VAR|UNUSED|THIS|CV, CONST|TMPVAR|UNUSED|NEXT|CV, SPEC(DIM_OBJ), binary_op_type binary_op) | ||
| ZEND_VM_INLINE_HELPER(zend_binary_assign_op_helper, CONST|TMP|VAR|UNUSED|THIS|CV, CONST|TMPVAR|UNUSED|NEXT|CV, SPEC(DIM_OBJ), binary_op_type binary_op) |
There was a problem hiding this comment.
How it's possible to assign to IS_CONST or IS_TMP_VAR?
There was a problem hiding this comment.
You cannot assign to these… It's just the syntax of ASSIGN_STATIC (ClassName::$foo - ClassName and foo are both CONST)
|
We discussed the patch with @bwoebi some time ago, but didn't come to consensus. But I understood, the main reason of the change - is a preparation for typed properties. |
That's why I'm interested in merging it, a complete impl of typed properties is difficult/impossible/impractical without it. What kind of degradation are we talking about, is it the kind that is acceptable, or not ? |
|
@dstogov comment above was aimed at you, forgot to ping :) |
|
I can't remember digits. I'll continue working with Bob on typed properties on next week.
…________________________________
From: Joe Watkins <notifications@github.com>
Sent: Friday, January 13, 2017 10:00:33 AM
To: php/php-src
Cc: Dmitry Stogov; Mention
Subject: Re: [php/php-src] Separate static property assignment out into dedicated assign ops (#2226)
@dstogov<https://github.com/dstogov> comment above was aimed at you, forgot to ping :)
-
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#2226 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACZM0jiq5KW6SuMEj-wGWdpopZz96XtYks5rRyERgaJpZM4LBuEM>.
|
dstogov
force-pushed
the
extra_static_prop_assign_ops
branch
from
e3fb9a9 to
3d84aef
Compare
|
I guess it was merged via 3d84aef ? |
|
no, only part of this |
|
A variant of this change is part of typed properties in #3313, so closing this PR. |
6 participants
Uh oh!
There was an error while loading. Please reload this page.