On Mon, May 31, 2021 at 4:04 PM Go Kudo <zeriyo...@gmail.com> wrote: > Hi Internals. >
Hi :) Thanks for your continued work on this RFC. This is a tough one, with a lot of feedback, some of it contradictory. I'm still going to add my 2c... I think that this proposal can basically work out in two ways: 1. Pragmatic: Have a Random class that represents both the raw randomness source and ways to use it. 2. Pure: Separate the randomness source from transformations based on it. Your current design sits in a weird middle ground between these, where you can extend the Random class to insert a user-provided randomness source. I think you should pick between one or the other option. If you pick the first option, then user-extensibility should simply not be supported at all. The class should be final, and only support a limit set of extension-provided RNGs. This is a pragmatic design that covers most use-cases. If you pick the second option, then you should consistently separate the source for both extension-provided RNGs and user-provided ones. I don't think it makes sense to have extension provided RNGs use a `new Random(RANDOM_XORSHIFT128PLUS)` interface, while user-provided ones use `new Random(new SomeRandomSource)`. Going down this route, it should be `new Random(new XorShift128Plus)`. This is a pure design, which also means that it's more complicated, and may make simple usages harder (though there is certainly room for a Random::createDefault() here.) As a minor note, I don't think we should add something like random.ignore_generated_size_exceeded. Our general policy is to avoid the introduction of ini settings that affect runtime behavior where possible. We should pick one of the behaviors here and stick with it. Regards, Nikita I apologize for the discussion outside the ML. Here's a brief history. > > https://github.com/php/php-src/pull/7079 > > - To test the implementation, I sent a Pull-Request to php-src on GitHub. > - An excellent point was made, and we ended up having a discussion about it > on GitHub. > - I was going to change the current implementation RFC to Under Discussion, > but decided against it. > > Also, sorry for the delay in answering your questions. I am not very good > at English and it is taking me a long time. > > The current implementation and RFC: > > - https://github.com/php/php-src/pull/7079 > - https://wiki.php.net/rfc/rng_extension > > However, the implementation of this user-defined class is very ugly and we > intend to improve it as follows. > > Before: > ```php > > class Random > { > public function __construct(string $algo = RANDOM_XORSHIFT128PLUS, ?int > $seed = null) {} > //.... > } > > class UserDefinedRandom extends Random > { > protected function next(): int > { > return 1; > } > } > ``` > > After: > ```php > interface RandomNumberGenerator > { > public function generate(): int; > } > > > final class Random > { > public function __construct(string|RandomNumberGenerator $algo = > RANDOM_XORSHIFT128PLUS, ?int $seed = null) {} > } > ``` > > 2021年5月26日(水) 0:35 Go Kudo <zeriyo...@gmail.com>: > > > Hi, Thanks for the response. > > > > The RFC has been revised based on the points you pointed out. > > > > https://wiki.php.net/rfc/rng_extension > > > > The main changes are as follows: > > > > - Class name has been changed from `Randomizer` to `Random` . > > - Added a static method `getNonBiasedMax()` to get the safe range. > > - `int()` and `bytes()` have been renamed to `getInt()` and `getBytes()` > > for avoid future reserved words. > > - `getInt()` arguments no longer accept null. > > - `shuffle(array|string $target): array|string` has been separated into > > `arrayShuffle(array $array)` and `stringShuffle(string $string): string` > > for more comfortable static-analysis. > > - fix: php_random_algo struct (included uint64_t -> int64_t) > > > > Answer a few questions: > > > > > When $seed is null, what is used for the seed value? > > > > Depends on the algo's implementation, but basically it is using internal > > `php_random_int()`. > > It is similar to `mt_srand()` on PHP 8.1. > > > > > > > https://github.com/php/php-src/commit/53ee3f7f897f7ee33a4c45210014648043386e13 > > > > > Why cancelled RNG Extension? > > > > As a result of discussions during the draft, the functions became a > single > > class and no longer need to be separated. The functionality for random > > numbers is now included in ext/standard and will conform to this. (e.g. > > rand.c random.c) > > > > > Deprecation > > > > Dropped in the last RFC update. > > These were premature and should not have been included with the RFCs that > > add new features. > > > > If the direction seems generally okay, I'd like to start implementing it > > to show more details. > > > > Regards, > > Go Kudo > > > > 2021年5月23日(日) 5:56 Go Kudo <zeriyo...@gmail.com>: > > > >> 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 > >> > > >
