Closed marc-mabe closed 7 years ago
Question:
As far as I understand Enum::set($fill = false)
should return an EnumSet instance (dependend on how many values are present and Integer- or BinaryEnumSet. But what does the $fill
parameter do? Add the existing Enum to the set?
Another method name proposal would be: Enum::createSet($fill = false)
@prolic
But what does the
$fill
parameter do?
Sorry I didn't explained this part. The idea of the $fill = false
parameter is to create an empty set by default and if set to true to attach all enumerators internally.
Another method name proposal would be: Enum::createSet($fill = false)
I like this :+1:
And in general do you like this idea of having two different EnumSet
implementations based on need?
Yes, some docs are needed in order to explain the difference, but I see no problem. IntegerEnumSet will be enough for most cases and BinaryEnumSet only fallback when you have too many values. Sounds good then.
Great - thanks for your input.
I just have two very detailed questions:
1.) Should the binary bitset functions [get|set]BinaryBitset[Le|Be](string $bitset)
be available on IntegerEnumSet
and be part of the abstract class? Just because it's possible to (un-)pack integers into binary strings and to reduce BC break.
2.) Should the functions [get|set]Bit(int $ordinal, bool $bit)
be public or protected. Currently I documented it as protected just because it feels like an implementation detail but you could use this function to attach/detach an enumerator by ordinal number much faster then EnumSet::attach(MyEnum::getByOrdinal($ordinal))
.
to 1.) Yes why not? I like everything that helps to reduce the amount of client coded needed. If this library helps with this, it's a good thing.
to 2.) same answer - it can be a useful addition and helps to reduce boilerplate on the userland code, so very good for me.
fixed in #86
An integer bitset would drastically improve performance but can handle only up to 64 enumerators on 64 bit platform and up to 32 enumerators on 32 bit platform.
Both implementations should extend the
EnumSet
class which would be abstract:We could also add a helper method to instantiate an
EnumSet
based on the number of enumerators but here I'm totally unsure of the name:Enum::getEnumSet($fill = false)
orEnum::getSet($fill = false)
orEnum::set($fill = false)
or ?Thoughts?
@prolic