Messages in this thread | | | Date | Tue, 15 Nov 2022 09:42:25 +0100 | From | Yann Droneaud <> | Subject | Re: [PATCH v1 3/5] random: add helpers for random numbers with given floor or range |
| |
Hi,
Le 14/11/2022 à 19:38, Jason A. Donenfeld a écrit : > On Mon, Nov 14, 2022 at 07:04:13PM +0100, Yann Droneaud wrote: >> I have a bad feeling about this one, and can't help but thinking it's going >> to bite someone: when asked to pick a number *between* 0 and 10, >> I usually think I'm allowed to pick 10 (even if I'm going to answer 7 as it should). > This is one of those bikeshed things you see all over the place, like > whether slices in a language should be [start index, end index] or > [start index, length], or whether arrays should be 0-based or 1-based. > We'll never settle this variety of dispute here. > > But in this case, there are some particular reasons why it must be this > way. Firstly, usage of it this way matches most of the ways the function > is actually used in the kernel, and fits existing semantics. This alone > I find compelling. But also, having all of these functions use half-open > intervals means that each function can take care of its entire range, > without having to resort to using 64-bit arithmetic, and no function is > a complete subset of any other function. So doing it this way makes > these maximally useful too.
For get_random_below(), which replaces a modulo, <bikeshedding> and could have been called get_random_mod()</bikeshedding>, having an open upper range seems fine. It's already what can be achieved by the % operator.
But I believe it's unfortunate get_random_between() cannot be called to get a number up to UINT32_MAX, as get_random_between(0, UINT32_MAX) would be capped to UINT32_MAX - 1.
When not a constant, one could hope the function can cope with a maximum that would grow up to and including UINT32_MAX.
> So anyway I think the function has to be defined like this. If you'd > like to bikeshed over a different name than "between", though, be my > guest. Maybe you'd like "from" better. But probably "between" is fine, > and with enough good examples (as my conversion patch does) and the > clear succinct documentation comment, we should be good.
That the conversion patch [1] that triggered my comment: I find replacing the following rather unpleasing, somewhat uncanny:
-get_random_u32_below(1024) + 1 + get_random_u32_between(1, 1024 + 1) I would prefer - get_random_u32_below(1024) + 1 + get_random_u32_between(1, 1024) [1]https://lore.kernel.org/lkml/20221114164558.1180362-4-Jason@zx2c4.com/
Regards.
-- Yann Droneaud OPTEYA
| |