Hi
On 10/16/23 10:34, Christian Schneider wrote:
Am 15.10.2023 um 19:24 schrieb Tim Düsterhus <t...@bastelstu.be>:
Making getFloat(float $min = 0.0, float $max = 1.0, IntervalBoundary $boundary =
IntervalBoundary::ClosedOpen) would seemingly make it legal to call
->getFloat(0.5), which I consider to be worse than nextFloat().
While I understand that you find getFloat(0.5) undesirable I would not consider
it illegal, e.g. getFloat(max:5).
Additionally I would consider getInt(max:100) to be a very valid usage too.
I don't, because I prefer explicit calls. If the upper bound is
specified explicitly, the lower bound should be as well. In fact the
`0,` is even shorter than spelling out `max:`.
Side note: The implementation of nextFloat() is much more efficient than that
of getFloat(0.0, 1.0) and the output will differ even for the same seeded
engine. The domain of legal return values is identical.
This sounds like an implementation detail as I'm pretty sure you could switch
to that fast path easily enough if neither of the arguments were given.
Yes, that's why it's a side note.
Side-note: The case of nextInt() is a bit trickier and strikes me as a bit of a
weird API and I'd consider dropping it too: Having the max value depend on the
Randomizer engine makes the generated values unstable in regards to switch the
Engine. And if I read the documentation correctly then it only generates 32 bit
values (or should it be 31 bits as it returns positive values only?). I think
it would be clearer to require the Engine to provide at least 4 bytes and then
specify the default max value of getInt() to be 2^31, optionally overridable
with something up to PHP_INT_MAX.
Yes, Randomizer::nextInt() is terrible. Let me provide the context why
it exists like it exists:
1. The Randomizer was designed to be drop-in compatible with the
existing randomness functionality (mt_rand, array_rand, shuffle,
strshuffle) when combining it with the Mt19937 engine.
2. Thus mt_rand is equivalent to Randomizer::getInt().
3. It is legal to call mt_rand() without parameters.
4. This behavior was carried over into getInt().
5. During the final review, it was noticed that this is an overloaded
function and a decision was made not to do this, because overloaded
functions are terrible.
6. Thus the getInt()-without-parameters call was split into nextInt().
I think that the goal of making the new API a drop-in replacement has
been a useful goal to make it easier to migrate and I assume that's why
no one questioned whether retrieving "an arbitrary positive integer" is
even useful in the first place. While writing the documentation I first
realized how useless that is, because I was unable to write something
useful for Randomizer::nextInt(), but at that point it was way too late.
Hindsight is 20/20.
Given that knowledge, is naming `nextFloat()` like `nextInt()` a
mistake? I'm not sure. It was the obvious naming choice to me back when
I wrote the RFC a year ago and no one pointed out how the name was not
ideal, not during the RFC discussion phase, nor during the vote or in
the half year leading up to the feature freeze. Now 6 weeks before the
gold release, with 4 release candidates released and several blog posts
written, the method name suddenly is a bad choice?
So in summary I'd support both adding default values to getInt()/getFloat() as
well as dropping nextFloat() in favor of getFloat() (and possible nextInt()
with getInt()).
I'd be on board with deprecating nextInt() in PHP 8.4. I'd also be on
board with creating a better-named alias for nextFloat(), but I believe
that it should remaing a standalone method, because of its usefulness.
Best regards
Tim Düsterhus
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
