Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:112738
To: Ilija Tovilo <tovilo.ilija@gmail.com>,
 php internals <internals@lists.php.net>
References: <eadd4a32-a05b-463b-a91f-a5d353c577b2@www.fastmail.com>
 <67c9d05a-f0a7-fcea-32ea-7a4e409308a1@mabe.berlin>
 <5120eee2-56c4-4608-9e41-4dd05563fef7@www.fastmail.com>
 <ae91f274-96bd-0041-43da-d5eb7a9a8986@mabe.berlin>
 <CAPyj-LAkF_m8ZtnNTsxOSQh3B4zscevp-hmiCU-=1YBK35wcGg@mail.gmail.com>
Message-ID: <964326f1-a3ba-e343-86a6-ea88c33d45c8@mabe.berlin>
Date: Sun, 3 Jan 2021 21:25:43 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <CAPyj-LAkF_m8ZtnNTsxOSQh3B4zscevp-hmiCU-=1YBK35wcGg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Subject: Re: [PHP-DEV] [RFC] Enumerations, Round 2
From: marc@mabe.berlin (Marc)

Hi Ilija,

On 03.01.21 12:54, Ilija Tovilo wrote:
> Hi Marc
>
>> I don't have a really good use-case for float values. It just seems
>> weird to me that a ScalarEnum doesn't support all scalars.
>>
>> Using the enum value as array key for `cases()` works with your current
>> proposal but if we later want to allow floats, bool whatever then we got
>> a food gun.
> The main reason is that we're using a hashmap internally in from() to
> find the given case you're looking for. This is the same hashmap PHP
> arrays are based on which only supports ints/strings as keys. If we
> were to allow any scalar as a value, looking up a case by value would
> become a O(n) operation.w
>
> We could do something terrible like serialize the key before storing
> it in the hashmap to allow arbitrary key types. But that will require
> serializing the value on each invocation of from() which will
> unnecessarily slow down the 95% most common use cases (int/string) to
> support the exception. Note though that it's always easier to extend
> than to remove. By not offering this feature we're erring on the side
> of caution.
>
> That being said, I can see how ScalarEnum is a misleading name. We've
> been thinking about a better name and only had some ideas we weren't
> fully satisfied with. RawEnum, ValueEnum and ConvertibleEnum were some
> of these ideas. Let us know if you have a better suggestion.

That's reasonable and makes sense at this point. Only supporting string 
and int is also fine (I don't have a personal use case for other types).

So using the same HT implementation as arrays internally totally makes 
sense but this is an implementation detail not visible for the outside 
and we shouldn't block outself for the future now as nobody knows of 
unknown possible use cases. At least if we can avoid it.


>> You already provide a lookup mechanism with `MyEnum::from()` - I don't
>> see a real use-case for proving a pre build map. The main use case I see
>> is to list all possible enum values but this doesn't require a map and a
>> zero-indexed-array would also be more performant with packed arrays
>> (correct me if I'm wrong).
> I do somewhat agree with you there. We're essentially returning
> `Array<UnitEnum>|Map<int|string, ScalarEnum>` which feels
> inconsistent. When you're calling cases() you're most likely going to
> loop over it at which point $case->value is available at your
> disposal.

Would you consider making `cases()` returning a simple list in all cases 
instead of differentiate between UnitEnum and ScalarEnum given the fact 
that mostly people just want to loop over cases and a lookup is already 
available with ScalarEnum::from() to provide a cleaner interface?

Marc

> Ilija