Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:110681
MIME-Version: 1.0
References: <CAF+90c8BeG9GwQOVypcNe3p5Yp9LefsWHfH5Wrjq1o6HZvrZVQ@mail.gmail.com>
 <CAF+90c_mfx3fbwWGusKH+LdXcMV7g-EiX5H15-AAWmhPSxnLAA@mail.gmail.com> <CAF+90c__FQ3FQXTDTeJ8cV9Nhwni0XUGBf4WeaTRXkDzWasCRw@mail.gmail.com>
In-Reply-To: <CAF+90c__FQ3FQXTDTeJ8cV9Nhwni0XUGBf4WeaTRXkDzWasCRw@mail.gmail.com>
Date: Fri, 19 Jun 2020 12:32:47 +0200
Message-ID: <CAF+90c_7ZjbuoFZ5-0zCv=-KBycTJo4+AMh3AEqj_=MiKtLBTQ@mail.gmail.com>
To: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000fd25c705a86d6aa8"
Subject: Re: Making all Traversables an Iterator or IteratorAggregate
From: nikita.ppv@gmail.com (Nikita Popov)

--000000000000fd25c705a86d6aa8
Content-Type: text/plain; charset="UTF-8"

On Tue, Jun 9, 2020 at 3:33 PM Nikita Popov <nikita.ppv@gmail.com> wrote:

> On Tue, May 12, 2020 at 10:26 AM Nikita Popov <nikita.ppv@gmail.com>
> wrote:
>
>> On Wed, Mar 11, 2020 at 10:50 AM Nikita Popov <nikita.ppv@gmail.com>
>> wrote:
>>
>>> Hi internals,
>>>
>>> Userland classes that implement Traversable must do so either through
>>> Iterator or IteratorAggregate. The same requirement does not exist for
>>> internal classes: They can implement the internal get_iterator mechanism,
>>> without exposing either the Iterator or IteratorAggregate APIs. This makes
>>> them usable in get_iterator(), but incompatible with any Iterator based
>>> APIs.
>>>
>>
>> This should have said: "This makes them usable in foreach(), but
>> incompatible with any Iterator based APIs."
>>
>> A lot of internal classes do this, because exposing the userland APIs is
>>> simply a lot of work. I would like to add a general mechanism to make this
>>> simpler: https://github.com/php/php-src/pull/5216 adds a generic
>>> "InternalIterator" class, that essentially converts the internal
>>> get_iterator interface into a proper Iterator. Internal classes then only
>>> need to a) implement the IteratorAggregate interface and b) add a
>>> getIterator() method with an implementation looking like this:
>>>
>>> // WeakMap::getIterator(): Iterator
>>> ZEND_METHOD(WeakMap, getIterator)
>>> {
>>>     if (zend_parse_parameters_none() == FAILURE) {
>>>         return;
>>>     }
>>>     zend_create_internal_iterator_zval(return_value, ZEND_THIS);
>>> }
>>>
>>> This allows internal classes to trivially implement IteratorAggregate,
>>> and as such allows us to enforce that all Traversables implement Iterator
>>> or IteratorAggregate.
>>>
>>
>> Does anyone have thoughts on this change? Mostly this is a feature for
>> extensions, but also user-visible in that a bunch of classes will switch
>> from being Traversable to being IteratorAggregate.
>>
>> We may also want to convert some existing Iterators to
>> IteratorAggregates. For example SplFixedArray currently implements
>> Iterator, which means that it's not possible to have nested loops over
>> SplFixedArray. We could now easily fix this by switching it to use
>> IteratorAggregate, which will allow multiple parallel iterators to work on
>> the same array. Of course, there is BC break potential in such a change.
>>
>> There's some bikeshed potential here regarding the class name. I picked
>> "InternalIterator" as an iterator for internal classes, but "internal
>> iteration" is also a technical term (the opposite of "external iteration"),
>> so maybe that name isn't ideal.
>>
>
> Unfortunately this bikeshed remains unpainted... The proposed names were:
>
>  1. InternalIterator
>  2. ZendIterator
>  3. IteratorForExtensionClassImplementations
>  4. EngineIterator
>
> I'm somewhat partial to the third option, with a less verbose name:
> IteratorForExtensions.
>

I went ahead and changed the implementation to use IteratorForExtensions.
Is anyone overly unhappy with that one?

@Michal: "ExtensionsIterator" to me sounds like an iterator that iterates
over extensions.

Regards,
Nikita

--000000000000fd25c705a86d6aa8--