Re: [PATCH 1/1] zsh/random

["Jun. T" <takimoto-j@xxxxxxxxxxxxxxxxx>]

# Please do not separate a patch into [0/1] and [1/1].
# Some people reply to [0/1], others to [1/1].

What is the current status of this patch?

Here are my comments:


> 2024/04/28 4:31, Clinton Bunch <cdb_zsh@xxxxxxxxxxx> wrote:

> Doc/Zsh/mod_random.yo

> +item(tt(SRANDOM)) (
> +A random positive 32-bit integer between 0 and 4,294,967,295.

'between 0 and 4,294,967,295'
I think $SRAND can be 0; can it be 4,294,967,295, or only to
4,294,967,295-1?

> +item(tt(zrand_float+LPAR()RPAR())) (
> +Returns a random floating point number between 0 and 1.

'between 0 and 1' Is this inclusive?
Can zrand_float() return 1?
  
> +item(tt(zrand_int)+LPAR()tt(upper), tt(lower), tt(inclusive)RPAR()) (
> +Returns a random integer between tt(lower) and tt(upper). All parameters are
> +optional.  If none are specified it is equivalent to
> +tt(SRANDOM).

> +tt(inclusive) is a flag that controls whether the result is ever equal to
> +tt(upper).  By default it is not.

If 'inclusive' is 0 (off) by default, then
0 <= zrand_int() < 4,294,967,295 by default.
Is this equivalent to $SRANDOM?


> Src/Modules/random.c

> +#include <stdbool.h>
> +#include <stdint.h>

I guess stdbool.h is not used?

I think including stdint.h (new in C99 but may exist in earlier
compilers) without #ifdef HAVE_STDINT_H is OK now, since we
are now using C99 as the C language standard, right?

> +/* buffer to pre-load integers for SRANDOM to lessen the context switches */
> +uint32_t rand_buff[8];

static uint32_t rand_buff[8];

> +int
> +boot_(Module m)

UNUSED(m)


> Src/Modules/random.mdd

> +load=yes

Does this module really need be autoloadable? I don't know what are
the criteria that a module should be autoloadable.


> Test/V15random.ztst

I don't know whether zsh need to check the "quality" of the random
number. I guess the simple $RANDOM would also pass these tests.

Maybe better to check that $SRANDOM is 'non-repeatable'?

If we will do the quality check, then:

> +  print -ru $ZTST_fd 'This test may take two seconds...'

Why two seconds? A few seconds, or something like that?

> +  function calc_chi2() {
> +# Calculate Chi Squared
(snip)
> +       #echo
> +  }

Please remove several '#echo' (for debugging?).

You can convert the functions calc_chi2(), ligf() etc. into
math-functions by using 'functions -M'. Then you can call them as,
for example (( p = 1 - ligf(s, z)/gamma(s) ))

> +0q:random integer $samples samples between 1-$n

The flag 'q' works only for the lines starting with < > or ?.
$samples and $n are not expanded.

--
Jun