php-ds / ext-ds

An extension providing efficient data structures for PHP 7
https://medium.com/p/9dda7af674cd
MIT License
2.11k stars 95 forks source link

Collection implements Traversable. Wny not Iterator? #141

Closed githubeing closed 5 years ago

githubeing commented 5 years ago

You state that you intentionally made all classes final to enforce composition over inheritance. Map implements Traversable. I want to make a class that will contain a Map and a Generator and that will somehow iterate over them (how exactly - isn't important here). But it's currently impossible because Map implements Traversable and not Iterator. This means I may foreach it, but I cannot iterate it on a single step forward (next(), current()). I think this hinders me from using Map in composition. Could you implement it?

Why at all did you decide that implementing Traversable but not implementing Iterator is a good idea? (so that one could foreach all Map at a time but could not do it step by step) What is the purpose of this decision?

enumag commented 5 years ago

How about using https://www.php.net/manual/en/class.iteratoriterator.php?

rtheunissen commented 5 years ago

Map implements Traversable because it is not an iterator in itself. You can use an IteratorIterator to create an Iterator operating on the Traversable map.

Keep in mind that your iterator will be affected by future updates to the data structure. I am working on improving this in 2.x but I see no reason why we can not apply a version of that to 1.x as well.

Please see https://github.com/php-ds/ext-ds/issues/17

githubeing commented 5 years ago

@enumag , @rtheunissen , thank you!

did you try that? i've just tried and got an interesting error:

<?php
use Ds\Map;
$map = new Map([1=>1,2=>2,3=>3]);
$i = new IteratorIterator($map);
//$i->rewind();
var_dump($i->valid());
var_dump($i->current());
$i->next();

when the execution gets to the next() call, the php stops abnormally. browser (chromium) reports:

This page isn’t working server didn’t send any data. ERR_EMPTY_RESPONSE

in apache error_log (i use xampp on ubuntu) i see the following:

[core:notice] [pid 11678] AH00051: child pid 23067 exit signal Segmentation fault (11), possible coredump in /opt/lampp

(didn't find any dump in /opt/lampp btw)

if you uncomment the rewind() line, the error doesn't occur, everything works fine.

rtheunissen commented 5 years ago

I'll look into that segfault but I think you have to call rewind before you can iterate.

githubeing commented 5 years ago

cannot find in php docs that one MUST call rewind first. could you point to such statement? Other php traversables won't produce any error if you won't call rewind, afaik

anyway, even if it produces an error, this has to be a php error, throwable of something, of couse, not segfault obviously

githubeing commented 5 years ago

i only found a

Note: This is the first method called when starting a foreach loop. It will not be executed after foreach loops. https://www.php.net/manual/en/iterator.rewind.php

it states only that foreach DOES call rewind, not that everyone MUST call it

nikic commented 5 years ago

@githubeing At least for many SPL iterator calling rewind() is necessary. Sometimes iterators may work without it, but if they do, that's just a lucky accident.

(Of course there should never be a crash due to a missing rewind() call, but there may be misbehavior.)

githubeing commented 5 years ago

i'm convinced that such things (e.g. such as required order of method calls (such that if you don't follow it, you'll get an error)) have to be explicitly stated in the docs.

imo one of the bad sides of php itself (not ds ext) is the lack of consistent strictly defined contracts on which a dev could rely

when something that should fail still works for some magic reason, then tomorrow it may suddenly stop working and you'll never now why (cause you didn't know why it worked). if rewind MUST be called first, then any access to any other method MUST produce error if rewind hasn't been called. and if one of them produce such error while others somehow continue to work, this is very confusing, agree? php is getting better as time goes by, but still has this disadvantage in some cases.

githubeing commented 5 years ago

therefore, i propose to state explicitly at least in ext-ds php docs that rewind has to be called first.