On Mon, Jun 22, 2020 at 5:56 PM Björn Larsson <bjorn.x.lars...@telia.com> wrote:
> Hi Nikita, > > Den 2020-06-19 kl. 12:32, skrev Nikita Popov: > > On Tue, Jun 9, 2020 at 3:33 PM Nikita Popov <nikita....@gmail.com> > wrote: > > > >> On Tue, May 12, 2020 at 10:26 AM Nikita Popov <nikita....@gmail.com> > >> wrote: > >> > >>> On Wed, Mar 11, 2020 at 10:50 AM Nikita Popov <nikita....@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 > > I'm actually in favour of the term InternalIterator like you first > proposed. Internal and external iteration is clearly something > different, so no need due to these to shy away here ;-) > > And today this iterator will be for extensions, but if somehow > that would change in the future (which I don't think), option > 3 is not ideal. > Okay ... I've now merged this using the original InternalIterator name. Nikita
