On Wed, Jun 9, 2021 at 3:16 PM Go Kudo <zeriyo...@gmail.com> wrote: > Thanks Guilliam. > > > I'm not sure you really addressed > https://externals.io/message/114561#114680 ? > > I thought I had solved this problem by implementing the > `RandomNumberGenerator` interface. > > Accepting strings and objects as arguments is certainly complicated, but I > thought it met the necessary requirements. > From the aspect of static analysis, there seems to be no problem. > > Reverting to a full object-based approach would increase the cost of > providing new RNGs from extensions. I think the string approach is superior > in this respect. >
How much more "costly" would it be to define a class (implementing RandomNumberGenerator) and use its (full) name as the algo identifier? > Nikita's `createDefault()` approach is certainly good, but I think it is > still difficult for beginners to understand. > > I also thought about it for a while after that, and I think that the > current implementation is the most appropriate for now. What do you think? > I still agree with Nikita that there's a discrepancy between e.g. `new Random(RANDOM_XXX, $seed)` and `new Random(new CustomRNG(/*custom args*/))`, which also poses additional questions, e.g.: - (already pointed out by Jordi) `new Random(new CustomRNG(/*custom args*/), $seed)` is "illogical" (the passed $seed won't be used) but "possible" (even if your current implementation throws a ValueError when $seed is non-null); - This opens the possibility of "leaking" the RNG "source" (even if no `public function getRNG()`), e.g.: ``` $rng = new CustomRNG(/*custom args*/); $r1 = new Random($rng); $r2 = new Random($rng); var_dump($r1->nextInt() === $r2->nextInt()); // false (not true), I suppose? ``` or: ``` $rng = new CustomRNG(/*custom args*/); $r = new Random($rng); var_dump($r->nextInt()); // 1st of sequence $rng->generate(); var_dump($r->nextInt()); // 3rd of sequence (not 2nd), I suppose? ``` (possibly in less obvious ways), which may be considered bad or not (but still a discrepancy vs extension-provided algos). Again, taking a class name and optional extra args (or an $options array, like password_hash(), maybe even including 'seed' only for the algos that need one) to construct, rather than an already constructed object, might be better? But that would also "split" the constructor args from the class (increasing the risk of "mismatch"), and make using anonymous classes less easy, and maybe we haven't considered all the uses cases (e.g. what about `Random::getAlgoInfo(CustomRNG::class)`?)... So that might also be worse :/ It would be great to have more insights! (if nobody else speaks up then let's keep it as is of course) > > Couldn't the Random class simply implement `public function __clone(): > void` (that would internally do like `$this->rng = clone $this->rng;`)? > > Implicitly cloning in areas where the user cannot interfere may cause > problems when using objects that cannot be cloned. > However, this may be overthinking it. The reason is that a > `RandomNumberGenerator` that cannot be cloned should be implemented as algo > in the first place. > If there are no objections, I will change the implementation. > Ah, true, I hadn't thought about non-clonable implementations... But that's already the case for `__serialize()` and non-serializable implementations, right? (But let's wait a bit for possible objections, indeed) > > Indeed, a decision has to be made. If you throw an exception, we > wouldn't have the "issue" of different results on 32- vs 64-bit machines, > but XorShift128+ and Secure would be unusable on a 32-bit machine, right? > > I don't see any problem with the current implicit truncation behavior for > this. Even if a user switches to a 64-bit environment, compatibility can be > maintained by explicitly truncating the generated values. In addition, most > of the other methods only use 32bit internally. > If you are concerned about this, you should probably be concerned about > the incompatibility with MT_RAND_PHP. > That problem can also be compensated for with an extension that adds algo. > I thought you were "struggling with [this implicit truncation] behavior when calling nextInt() in a 32-bit environment using a 64-bit RNG [...] which means that the code loses compatibility with the result of running on a 64-bit machine"? And you asked if throwing an exception would be preferable? Anyway, I personally don't care about 32-bit (but other people may). > Regards, > Go Kudo > Regards, -- Guilliam Xavier
