Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114565
MIME-Version: 1.0
References: <CABTDVhDRH8+0WG0nJ5QRYVT9-8m1u2VMe-gvY5cWra-OKQ4t+g@mail.gmail.com>
In-Reply-To: <CABTDVhDRH8+0WG0nJ5QRYVT9-8m1u2VMe-gvY5cWra-OKQ4t+g@mail.gmail.com>
Date: Mon, 24 May 2021 16:46:09 +0200
Message-ID: <CAC5V6gi2e2UFWvC0x9t4VcR7H985q3mc3R0ryTwiYRvAbJAeCA@mail.gmail.com>
To: Go Kudo <zeriyoshi@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000e0a7ca05c31478bb"
Subject: Re: [RFC] [Draft] Add Randomizer class (before: Add RNG extension)
From: guilliam.xavier@gmail.com (Guilliam Xavier)

--000000000000e0a7ca05c31478bb
Content-Type: text/plain; charset="UTF-8"

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

--000000000000e0a7ca05c31478bb--