search

marc-mabe / php-enum

Simple and fast implementation of enumerations with native PHP
BSD 3-Clause "New" or "Revised" License
464 stars 36 forks source link

[WIP] EnumSet implements IteratorAggregate instead of Iterator #108

Closed marc-mabe closed 5 years ago

marc-mabe commented 5 years ago

This opens possibility to have two different iterators of the same EnumSet object. It could also reduce some memory on instantiating an EnumSet by the cost of having to instantiate the iterator separately if needed.

@prolic ping

prolic commented 5 years ago

@marc-mabe that's a great idea! By the way, your EnumSetTest are failing.

marc-mabe commented 5 years ago

@prolic Nice that you like the idea. I know that it was failing and it's still work in progress but all tests should be fixed now.

The reason was for how the behavior was on modifying the set in the middle it iteration. To fix that I had to create a reference of the bitset instead.

I'm actually not 100% sure what the best behavior would be.

  1. Copy (copy-on-write) the bitset which means the iterator does not change after calling getIterator
  2. Pointing (variable reference) to the bitset which means the iterator reflect the current state of the enum set at any point in time

From implementation PoV the first behavior seems more clear, less complex and even faster as of the way PHP handles references but from usage PoV I could think of both.

PS: If we rewrite the EnumSet to be immutable there would be no difference in behavior but I actually don't like to have an immutable EnumSet where the EnumMap is not possible to implement in an immutable way as of ArrayAccess.

What do you think?

prolic commented 5 years ago

I'm all in on immutability. As this is for the next major release, we could also drop ArrayAccess on EnumMap and make that immutable as well.

marc-mabe commented 5 years ago

@prolic Thanks for your input

For the iterator I will the the copy-on-write variant for now and if this works fine I'll try how it looks with an immutable set first before mergind but separate PR :)

Ah also I have changed Iterator::current() and Iterator::key() to throw an OutOfBoundsException in case of an invalid iterator position. This means the return type will not be nullable anymore - so no null checks just to make IDEs happy and accessing the current but invalid iterator position should be avoided anyway.

marc-mabe commented 5 years ago

closed in favor of #110