Fixed #33 #35
Fixed #33 #35
Conversation
|
@DaanBiesterbos WDYT? Just to note, this is a concept implementation. We still need to verify how it behaves with an empty enum, and it might be worth restricting such a case. Additionally, it might make sense to create a new class for this specific functionality. Documentation will need to be updated accordingly. I’ll take care of these steps, but I'm interested in your thoughts on the current implementation. |
|
I've found an issue when handling int-backed enums with non-sequential or random bit values. Specifically, the previous implementation assumed that enum values were sequential, which caused incorrect behavior when using enums like: enum RandomBackedInt: int {
case Bit1 = 1 << 0;
case Bit2 = 1 << 1;
case Bit3 = 1 << 3;
}In this scenario, the bitmask operations would fail because the enum values were not properly mapped. Additionally, we need to add a check to ensure that the IntBackedEnum uses correct bit values. I'll fix this. |
| * @throws UnknownEnumException | ||
| */ | ||
| public function __construct( | ||
| private readonly string $enum, | ||
| int $mask = 0, | ||
| bool $isIntBacked = false, |
There was a problem hiding this comment.
I noticed we instantiate the class in static functions. (like the all() method for example). We might run into consistency issues? It might be worth considering to treat all int backed enums as "isIntBacked". But that change would not be backward compatible, as people might be using int backed enum with the existing implementation.
That's why I was thinking about a subclass like IntBackedEnumBitmask.
There was a problem hiding this comment.
Thank you for your thoughts. You’re right that treating all int-backed enums as "isIntBacked" could lead to consistency issues and wouldn't be backward compatible. That's why the current approach includes the boolean parameter to ensure flexibility. However, creating a subclass like IntBackedEnumBitmask could be a cleaner solution in the long term.
| if (!is_int($case->value)) { | ||
| throw new InvalidEnumException('Enum must be an int-backed enum with integer values'); | ||
| } | ||
| } | ||
| $this->map[$case->name] = Bits::indexToBit($index); | ||
| } | ||
| $this->bitmask = new BitMask($mask, count($this->enum::cases()) - 1); |
There was a problem hiding this comment.
Not sure what the difference is, but I had an issue here with the most significant bit in my implementation. Not sure exactly why, but removing the second argument solved the issue for me. Perhaps you can speculate what could be the cause of this?
There was a problem hiding this comment.
Regarding the issue with the most significant bit, it’s possible that the problem stems from how the second argument (the mask) interacts with the bit positions in your enum. If the bitmask value overlaps or conflicts with the most significant bit, removing the second argument might have bypassed the issue. I’ll investigate further to pinpoint the exact cause.
No description provided.