Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:91948
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.161.179 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAESVnVqgMp6yeiZqWqk0b=72BJ3urcGcAXQmDmMm-GEWgSaBow@mail.gmail.com>
References: <CAESVnVqGWQbLRy9ctXh73=5X823F48HNAJuD_AZ=XBoFEfK1qg@mail.gmail.com>
	<CAESVnVpMFOOeHgoFxz6RmET_3sbHwd0H4jkogNWNFNsagD5Dww@mail.gmail.com>
	<CAESVnVqgMp6yeiZqWqk0b=72BJ3urcGcAXQmDmMm-GEWgSaBow@mail.gmail.com>
Date: Fri, 25 Mar 2016 14:44:44 +0100
Message-ID: <CAF+90c_-LiNWb_9NkvG4c4m_K9znzNoD-D72wU6xVgeTOq19AQ@mail.gmail.com>
To: Sara Golemon <pollita@php.net>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a114084a2fbc8ab052edfc3d0
Subject: Re: [PHP-DEV] Re: [RFC] [Vote] Short Ternary Assignment Operator
From: nikita.ppv@gmail.com (Nikita Popov)

--001a114084a2fbc8ab052edfc3d0
Content-Type: text/plain; charset=UTF-8

On Fri, Mar 25, 2016 at 4:25 AM, Sara Golemon <pollita@php.net> wrote:

> On Thu, Mar 24, 2016 at 11:12 AM, Sara Golemon <pollita@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

--001a114084a2fbc8ab052edfc3d0--