Re: Making all Traversables an Iterator or IteratorAggregate

This is only part of a thread. view whole thread
  110681
June 19, 2020 10:32 nikita.ppv@gmail.com (Nikita Popov)
On Tue, Jun 9, 2020 at 3:33 PM Nikita Popov ppv@gmail.com> wrote:

> On Tue, May 12, 2020 at 10:26 AM Nikita Popov ppv@gmail.com> > wrote: > >> On Wed, Mar 11, 2020 at 10:50 AM Nikita Popov 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
  110686
June 19, 2020 14:34 internals@lists.php.net ("Levi Morrison via internals")
> I went ahead and changed the implementation to use IteratorForExtensions. > Is anyone overly unhappy with that one?
I don't particularly care what color we paint this bike shed. The feature is valuable to me and look forward to it in PHP 8.
> @Michal: "ExtensionsIterator" to me sounds like an iterator that iterates > over extensions.
Agreed.
  110696
June 22, 2020 15:56 bjorn.x.larsson@telia.com (=?UTF-8?Q?Bj=c3=b6rn_Larsson?=)
Hi Nikita,

Den 2020-06-19 kl. 12:32, skrev Nikita Popov:
> On Tue, Jun 9, 2020 at 3:33 PM Nikita Popov ppv@gmail.com> wrote: > >> On Tue, May 12, 2020 at 10:26 AM Nikita Popov ppv@gmail.com> >> wrote: >> >>> On Wed, Mar 11, 2020 at 10:50 AM Nikita Popov 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
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. r//Björn L
  110712
June 24, 2020 13:32 nikita.ppv@gmail.com (Nikita Popov)
On Mon, Jun 22, 2020 at 5:56 PM Björn Larsson larsson@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 ppv@gmail.com> > wrote: > > > >> On Tue, May 12, 2020 at 10:26 AM Nikita Popov ppv@gmail.com> > >> wrote: > >> > >>> On Wed, Mar 11, 2020 at 10:50 AM Nikita Popov 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 > > 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