Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114534
MIME-Version: 1.0
References: <CABTDVhAd8r_OPsdbzxrFeZTZEHuZjKyPfcYhNcdfdSZWbUJ-Lw@mail.gmail.com>
 <CAC5V6giS2UFjHgPaiRiiMQKYEPxwR_34_vG6EQtMepgq7ReF4g@mail.gmail.com> <CABTDVhA=eK1V_EnZDJL7iiRdp5CK6prieVRNnZ6r0aRLxUoatQ@mail.gmail.com>
In-Reply-To: <CABTDVhA=eK1V_EnZDJL7iiRdp5CK6prieVRNnZ6r0aRLxUoatQ@mail.gmail.com>
Date: Thu, 20 May 2021 16:36:29 +0200
Message-ID: <CAC5V6giKT+yWNYPxX9d7-bHxhHOGQEP5x=MJB0nBC27gWdbGFQ@mail.gmail.com>
To: Go Kudo <zeriyoshi@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000001b497505c2c3df66"
Subject: Re: [PHP-DEV] [RFC] [Draft] Add RNG extension and deprecate mt_srand()
From: guilliam.xavier@gmail.com (Guilliam Xavier)

--0000000000001b497505c2c3df66
Content-Type: text/plain; charset="UTF-8"

On Thu, May 20, 2021 at 2:42 PM Go Kudo <zeriyoshi@gmail.com> wrote:

> Sorry. I couldn't make it in time for my lunch break.
>

No worries, there was quite a lot to read/reply ;)


> >  How many bytes is `Source::next()` supposed to generate? Is it even
> intended to be called directly (vs `Randomizer::generateBytes(int
> $length)`)?
>
> No, this is not intended to be used directly.
> In fact, as many people have pointed out, I think it would be more
> appropriate to merge it into a single class.
>
> As per the original implementation (ext/orng), I didn't think it needed to
> be separated originally. One of the comments on a previous RFC suggested
> that it would probably be better to separate them, and I took that into
> account.
>

I see. Sorry that you had so many back-and-forth suggestions :s I guess
that the simplest-to-use API (for the main intended/expected use cases) is
more likely to get acceptance.


> > About the `MT19937PHP` variant: The PHP manual for `mt_srand()`
> describes the `MT_RAND_PHP` mode like this: "Uses an *incorrect* Mersenne
> Twister implementation which was used as the default up till PHP 7.1.0.
> This mode is available for backward compatibility."; do we really need/want
> to port it to this new extension?
>
> This is for backward compatibility. If it is not needed, we would like to
> eliminate the implementation itself.
> Well, to keep the language core clean, maybe this should be provided by an
> appropriate external extension.
>

Yes, I think that a new core extension for PHP 8.1 needn't [or even
shouldn't] provide BC for an incorrect implementation that is not used
anymore since PHP 7.1 (unless someone here argues?).


>
> - `PlatformProvided` feels like the "odd one out" here: its constructor
> [assuming same as previous RFC] doesn't take a seed, and it isn't
> serializable; I understand why, but the main point of the RFC is "RNG
> reproducibility", which this class cannot achieve.
>
> You're absolutely right. I would like to remove this.
>

*Caution though:* after your recent discussion with Rowan, if you remove
the "change shuffle()/str_shuffle()/array_rand()'s internal
RNG from mt_rand()-like to random_bytes()-like" part from the proposal,
then this class becomes potentially "useful" again, not for reproducibility
but for shuffling an array/string by a "truly random" [actually CSPRNG]
algorithm, isn't it?


>
> > Technical, about `Randomizer::generateFloat()`: Is the returned float
> guaranteed to be finite (i.e. never NAN nor +/-INF)? And may it ever return
> -0.0 (negative zero)? and even if not, is 0.0 twice as likely as other
> floats?
>   Also, no `generateFloat(float $min, float $max)` variant like for
> `generateInt()`?
>
> As a matter of fact, we believe that the functions provided by this class
> are sufficient for `mt_rand()`, `shuffle()`, `str_shuffle()`, and
> `array_rand()` equivalents. At most, the equivalent of `random_byte()`
> might be provided.
> I'll reorganize these.
>

Sorry I'm not sure to understand your answer here, are you planning to
remove generateFloat() and generateBool()?
Well if generateBool() is a shorthand for `(bool) generateInt(0, 1)` [or
`generateInt(PHP_INT_MIN, PHP_INT_MAX) >= 0`] then it's not necessary, but
maybe convenient?
As for generateFloat(), I didn't know if you intended something like
`(float) generateInt(PHP_INT_MIN, PHP_INT_MAX) / PHP_INT_MAX` or rather
"call generateBytes(8) then reinterpret those 64 bits as an IEEE 754 double
[in C, not PHP]"?
But it's true that I haven't seen many people requesting new
"random_bool()" and/or "random_float()" functions, so...

By the way, for `generateInt()` (without explicit $min/$max args), I assume
that $max defaults to PHP_INT_MAX, but does $min default to PHP_INT_MIN or
actually 0 [if it's like [mt_]rand() I guess that the answer is probably 0,
but that could be written clear in the RFC]?


>
> Thanks for the detailed remarks. Based on these, I would like to clean up
> the RFC.
>
> Regards,
> Go Kudo
>

Regards,

-- 
Guilliam Xavier

--0000000000001b497505c2c3df66--