On Fri, Mar 25, 2016 at 4:25 AM, Sara Golemon <poll...@php.net> wrote:
> On Thu, Mar 24, 2016 at 11:12 AM, Sara Golemon <poll...@php.net> wrote: > > After some discussion (and realizing the referenced implementation > > needed more work that a simple replacement of tokens), I've decided > > the close this vote, work with Midori on both implementations some > > more, and reopen at a later time with a complete implementation > > (possibly combined with ??=) > > > > https://github.com/php/php-src/compare/master...sgolemon:short-ternary.coalesce > > I've gone from scratch to implement this branch which doesn't > introduce any new opcodes, but it does add a new AST kind which is > compiled into something closely (but not quite) resembling a regular > short ternary. > > It cheats slightly by assuming that since child[0] comes from a > `variable` in the parser that zend_compile_var() on it will always > yield IS_CV/IS_VAR, and I've got an assert in to guard on that, but a > second set of eyes would be nice there. > > You'll notice there's a new runtime check in ZEND_JMP_SET to handle > the IS_INDIRECT case (which I see resulting from the dim/obj paths. > It's hidden behind an existing check for IS_VAR/IS_CV, so anything > producing a TMP won't hit it. Hopefully it's not too harsh. > > Based on feedback, I hope to add the ??= version of this > implementation as another commit and let Midori unify the two RFCs > into one. (unless someone objects to that) > I don't think the current implementation is entirely correct. In particular, it doesn't look memory-safe to me. You're doing an RW fetch on the LHS first, then evaluate the RHS expression and then ASSIGN to the result of the RW fetch. This means you're running user code between the RW fetch and the assignment to it. The code of the RHS expression may cause a reallocation of the buffer into which the INDIRECT points, making it a dangling pointer. Something like $a = [false]; $a[0] ?:= ($a[''] = 42); will probably result in valgrinds. Nikita
