Minor cleanup of some unused code in SPL

[alexdowad]

No description provided.

[alexdowad]

These flaky tests are a real pain. I must do something about them.

Travis failure is only on S390X:

========DIFF======== 011+ fgets() took more than 10 ms 3 times ========DONE======== FAIL Bug #69900 Commandline input/output weird behaviour with STDIO [ext/standard/tests/streams/proc_open_bug69900.phpt]

AppVeyor failure is something to do with MySQL:

Fatal error: Uncaught TypeError: mysqli_fetch_assoc(): Argument #1 ($mysql_result) must be of type mysqli_result, bool given

...Both utterly unrelated to this removal of unused code from SPL.

[nikic]

I've merged a part of these, not sure about the rest wrt a possible migration from Iterator -> IteratorAggregate. I guess for SplHeap that just isn't possible, because the iterator is actually destructive, so two iteration streams wouldn't ever make sense?

[alexdowad]

I've merged a part of these, not sure about the rest wrt a possible migration from Iterator -> IteratorAggregate. I guess for SplHeap that just isn't possible, because the iterator is actually destructive, so two iteration streams wouldn't ever make sense?

That's a very good point about SplHeap.

The API for that one really has me scratching my head. Not sure what the original implementer was thinking.

[alexdowad]

Is there any reason for keeping the flags fields in spl_heap_it and spl_dllist_it? I see you haven't merged those.

Please note that removing those fields is not something specifically related to the change to IteratorAggregate -- the fields are just literally unused. There is nothing anywhere in the codebase which reads them. They are just set and then ignored.

[alexdowad]

@nikic I am wondering if what you said above might apply to SplPriorityQueue as well...

SplPriorityQueue uses flags to determine whether it should return an item, a priority value, or an array of the two from current(), etc. Those flags are also used when iterating over the priority queue.

Since it currently ignores the flags on the iterator and just uses the flags on the pqueue object, it means that if ::setExtractFlags() is called in the middle of iteration, what is returned by the already-existing iterator will change...

But then again, I wonder if that's a bad thing. It's a bit hard to say which is better.

The direction of DLL iteration changing in the middle of a foreach loop is definitely strange though. I think that one should be fixed.

[alexdowad]

OK, fixed this up and force-pushed...

Please note that the constant IT_MODE_KEEP in SplDoublyLinkedList is not used anywhere. Would it be a good idea to remove it? Of course, this could break old code that still refers to it... :sad:

I personally feel that SplMinHeap et al do not need to use the (currently unused) flags field in the iterator. It means that if someone calls ::setExtractFlags() in the middle of a foreach loop, the loop will change from yielding keys to yielding priority values (or vice versa). But I don't think that's a bad thing.

However, if you feel that this behavior of SplMinHeap should be changed, that is fine. I can do that.

[alexdowad]

Ping...

I'd like to suggest that the changes to SplDoublyLinkedList here are ready for merging. As for SplMinHeap, perhaps more discussion might be needed.

[alexdowad]

I feel that SplDoublyLinkedList::IT_MODE_KEEP should be deprecated. I have seen how to mark a global constant as deprecated, but the commit message on 9ec1ee5 suggests that is not possible yet for class constants...

Maybe I should try to implement deprecation of class constants?

[nikic]

I cherry-picked the changes related to the SplDoublyLinked list fix.

Please note that the constant IT_MODE_KEEP in SplDoublyLinkedList is not used anywhere. Would it be a good idea to remove it? Of course, this could break old code that still refers to it... :sad:

The same goes for IT_MODE_FIFO though. Both of those are simply the default. I assume IT_MODE_KEEP exists as the counter to IT_MODE_DELETE, to explicitly specify the behavior. As such, I'm not completely convinced that it should be removed.

I personally feel that SplMinHeap et al do not need to use the (currently unused) flags field in the iterator. It means that if someone calls ::setExtractFlags() in the middle of a foreach loop, the loop will change from yielding keys to yielding priority values (or vice versa). But I don't think that's a bad thing.

Not sure on this part... But probably you're right.

Maybe I should try to implement deprecation of class constants?

Wouldn't hurt for sure ^^ Even if it's not immediately necessary, we can add a deprecated class constant to zend_test, because this will definitely come up in the future.

[alexdowad]

OK, force-pushing very different changes to spl_heap.c which make it actually use the iterator flags.

[alexdowad]

Also added a new unit test.

[alexdowad]

Please note that the constant IT_MODE_KEEP in SplDoublyLinkedList is not used anywhere. Would it be a good idea to remove it? Of course, this could break old code that still refers to it... :sad:

The same goes for IT_MODE_FIFO though. Both of those are simply the default. I assume IT_MODE_KEEP exists as the counter to IT_MODE_DELETE, to explicitly specify the behavior. As such, I'm not completely convinced that it should be removed.

Thanks for the good point. I actually came to a similar conclusion after thinking about it more.

[Girgias]

What's the state of this PR?

[github-actions]

There has not been any recent activity in this PR. It will automatically be closed in 7 days if no further action is taken.

[Girgias]

@alexdowad are you planning on having another look at this or should this be closed? (and instead of just commenting I pressed the close PR one...)

[github-actions]

There has not been any recent activity in this PR. It will automatically be closed in 7 days if no further action is taken.