Re: [RFC] [Draft] Add Randomizer class (before: Add RNG extension)

On Sat, May 22, 2021 at 10:57 PM Go Kudo <zeriyoshi@gmail.com> wrote: > Hi, Internals and all participated in the previous discussion. > > RFCs have been cleaned up and the proposal has been substantially changed. > > https://wiki.php.net/rfc/rng_extension > > First of all, I apologize for not checking out the implementation of > password_hash(), which is a precedent to learn from. > > I think I've answered all the questions I've been getting on Open Issues. > If I have left anything out, please let me know. > > Regards, > Go Kudo > Hi, This really looks like a different proposal indeed (simpler and clearer), thanks! A few (new) remarks/questions: - Naming: "Randomizer", but some have suggested just "Random", or "RNG" (or "PRNG"). - [I was about to say "If typical usage is expected to provide a seed (for reproducibility) more often than choosing a non-default algorithm, maybe the [optional] constructor parameters should be in the according order ($seed, $algo)?", but actually I'm not sure of "expected typical usage", and the current order ($algo, $seed) seems more "logical" (e.g. I think RANDOMIZER_SECURE will ignore $seed?), and we can use named arguments like new Randomizer(seed: 1234) if needed, so...] - Does ?int $seed = null default to time() internally? or to something else? [not sure if important...] - Does ?int $min = PHP_INT_MIN default to PHP_INT_MIN even if we pass null? But, given that we can use named arguments, do $min/$max really need to be nullable? - About shuffle(array|string $target): array|string: I think just "value" would be better than "target", and I'm not sure about the union type (notably for static analysis)... Ideally it should be distinct `(array $value): array and (string $value): string`, but that probably requires two distinct names? - For internal implementation, isn't there a signed/unsigned "mismatch" between PHP function next(): int and C uint64_t (*next)(void) return types? Regards, -- Guilliam Xavier