On Sat, May 22, 2021 at 10:57 PM Go Kudo <zeriyo...@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
