On Thu, May 20, 2021 at 2:42 PM Go Kudo <zeriyo...@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
