Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collection implements Traversable. Wny not Iterator? #141

Closed
githubeing opened this issue Apr 14, 2019 · 9 comments
Closed

Collection implements Traversable. Wny not Iterator? #141

githubeing opened this issue Apr 14, 2019 · 9 comments

Comments

@githubeing
Copy link

githubeing commented Apr 14, 2019

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
Copy link

enumag commented Apr 14, 2019

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

@rtheunissen
Copy link
Member

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 #17

@githubeing
Copy link
Author

@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
Copy link
Member

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

@githubeing
Copy link
Author

githubeing commented Apr 14, 2019

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
Copy link
Author

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
Copy link
Contributor

nikic commented Apr 14, 2019

@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
Copy link
Author

githubeing commented Apr 14, 2019

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants