Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:102912
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.216.174 as permitted sender)
MIME-Version: 1.0
References: <CAF+90c_L7S+EB8YxdNNpNZ7NhhYwCbwEw5_TSnSwUNN-zLVvCg@mail.gmail.com>
 <CAF+90c8ZG8FYy74i+HmGPb=YgsaT=CLRDqs0TxNo+X13AeyeTg@mail.gmail.com>
 <BL0PR02MB443503A3D9340A7A91E8AD5DAC5D0@BL0PR02MB4435.namprd02.prod.outlook.com>
 <CAF+90c9o3ou_QGGMAV=S6NeXeTB+-ew=um=+ZJocQWDhgvHDNw@mail.gmail.com>
In-Reply-To: <CAF+90c9o3ou_QGGMAV=S6NeXeTB+-ew=um=+ZJocQWDhgvHDNw@mail.gmail.com>
Date: Thu, 19 Jul 2018 11:52:37 +0300
Message-ID: <CAJHtznxDSYGuUoi3G_C=WsDtiShPV64Pjj_cvgpX82=7QvbOKQ@mail.gmail.com>
To: Nikita Popov <nikita.ppv@gmail.com>
Cc: Zeev Suraski <zeev@zend.com>, Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000c1e2cb0571564d14"
Subject: Re: [PHP-DEV] Re: [RFC] Typed Properties
From: vsuraski@gmail.com (Zeev Suraski)

--000000000000c1e2cb0571564d14
Content-Type: text/plain; charset="UTF-8"

On Tue, Jul 17, 2018 at 9:02 PM Nikita Popov <nikita.ppv@gmail.com> wrote:

>
> Sure, we can wait another week. Unless the additional discussion results in
> major changes to the RFC, we'll start voting next Monday (2018-07-23). In
> the interest of avoiding further delays, please try to view this as a hard
> deadline: If you would like to discuss some aspect of the proposal or raise
> a concern, please do so now rather than on Monday morning.
>

I reviewed the proposal in detail - and quite detailed it is.  It seems
like you and Bob did a very thorough job here, kudos on that.

I do have 3 comments - none of them major - but I think should be addressed
nonetheless:

1. The RFC is surprisingly sparse on examples where the typed properties
are not scalar ones.  So much, in fact, that I had to check whether this is
intentional (i.e., classes are unsupported as enforced property types) or
an oversight.  Reviewing the 'Supported Types' section, while mentioning a
ClassOrInterface - isn't entirely clear, as it's unclear whether this is a
token, or something that stands for any class name or interface.
Thankfully the proposal does cover class and interface names as property
type definitions.  I would recommend to be explicit here, and say "any
class or interface name", and ideally also provide a sample of that option
in the Introduction section of the RFC.
2.  While trying to understand whether #1 was in fact supported and
reviewing the patch, I noticed that there were several comments from Dmitry
on the patch (within the pull request).  It would be nice if they were
addressed (none of them will turn the patch upside down, and as I see it
they are orthogonal to the vote).
3. The patch does bring a performance penalty, albeit quite small (every
property assignment now has to conduct type safety checks).  This does not
only affect code that uses typed properties - but also code that doesn't.
I think this should be mentioned in the RFC.  To be clear - I don't think
the penalty is substantial enough to change someone's opinion about the
merits of the proposal one way or another - but in the spirit of full
disclosure it should be there.

Zeev

--000000000000c1e2cb0571564d14--