Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:103741
MIME-Version: 1.0
References: <CAF+90c-V9vG8eAanQgujb+GhkLWtH2N0VRe500oocNQ7ryfb=Q@mail.gmail.com>
 <CADyq6s+qnFfZGMfr0HfyW6Qu9MC+_M_h3mQDOiDMJzWWTVHE-Q@mail.gmail.com>
In-Reply-To: <CADyq6s+qnFfZGMfr0HfyW6Qu9MC+_M_h3mQDOiDMJzWWTVHE-Q@mail.gmail.com>
Date: Tue, 15 Jan 2019 11:57:20 +0100
Message-ID: <CAF+90c8u0Qn2+xJBpae+7YGBSXeaw_7RdZ6Ym7Q_R48ZWA7pLQ@mail.gmail.com>
To: Marco Pivetta <ocramius@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000007c8688057f7d0721"
Subject: Re: [PHP-DEV] [RFC] Reflection for references
From: nikita.ppv@gmail.com (Nikita Popov)

--0000000000007c8688057f7d0721
Content-Type: text/plain; charset="UTF-8"

On Tue, Jan 15, 2019 at 11:30 AM Marco Pivetta <ocramius@gmail.com> wrote:

> A few issues with the RFC so far:
>
>  * `ReflectionReference` seems to be designed around arrays only: maybe
> `ReflectionArrayKeyReference` or such?
>

ReflectionReference works with arbitrary references. Arrays only come in as
a detail of construction. Constructors such as fromLocalVariable() or
fromObjectProperty() could be added in the future under this design.


>  * Can the `getId()` return type be restricted to either `int` or
> `string`? Why is it a union type right now? Technical limitation?
>

To allow changes in the future. The real return value here is an int, but
the size increase due to hashing makes it a string. If we find a way to
avoid that (or decided that we don't care about leaking addresses), we
could change this to a (faster) int-based API.

I'd be okay with renaming this to getHash() and restricting the return
value to string. We could add getId() returning int in the future, same as
we did for objects.


>  * `ReflectionReference#__construct()` should be `private`, since it is
> unusable anyway
>

Done.


>  * `fromArrayElem` => `fromArrayElement`
>

Done.


>  * is this open for inheritance? If so, what scenarios would fit
> inheriting from `ReflectionReference`?
>

Marked as final.


>  * what happens when `ReflectionReference::fromArrayElement()` is given
> invalid data, such as non-existing keys? I'd expect it to throw.
>

It will throw, yes. Added to the RFC:

> If $array is not an array or $key is not an integer or string, a
TypeError is thrown. If $array[$key] does not exist, a ReflectionException
is thrown.


>  * instead of `$ref1->getId() === $ref2->getId()`, `$ref1->matches($ref2)`
> or such.
>
 * what are possible scenarios for getting the identifier as a primitive,
> and then storing it somewhere (like an array of reference identifiers)?
>
 * There seems to be a lot of design around `ReflectionReference#getId()`
> to avoid leaking internal pointer information: can it be completely
> dropped, if we have `$ref1->matches($ref2)` instead?
>

As Nicolas already wrote, the ID / Hash functionality is needed to allow
hashtable lookups. Having just matches() would require iterating over the
whole set of known references and comparison each and every one. With the
getId() API, it's just a single HT lookup.

We could add $ref1->equals($ref2) as an *additional* convenience method,
but it would not replace the getId() API.

Thanks for the feedback!
Nikita

--0000000000007c8688057f7d0721--