Re: [PATCH 1/1] zsh/random

[Clinton Bunch <cdb_zsh@zentaur.org>]

On 6/17/2024 09:43, Jun. T wrote:
> # 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?
It's been waiting on feedback
>
> Here are my comments:
>
>
>> 2024/04/28 4:31, Clinton Bunch <cdb_zsh@zentaur.org> 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?
4,294,967,295 is 2**32-1 (odd number)
>
>> +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?
Yes. it can return both 0 and 1.  I've specified inclusive in the 
documentation.
>    
>> +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?
In the code it calls the same function as SRANDOM when no arguments are 
given.
>
>> Src/Modules/random.c
>> +#include <stdbool.h>
>> +#include <stdint.h>
> I guess stdbool.h is not used?
Apparently I removed all places where I used a bool type
>
> 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.
I would think we'd want at least the SRANDOM parameter autoloaded
>
>
>> 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.
RANDOM would not pass these tests.  They test the difference between the 
distribution of the generated values against the ideal equal distribution.
>
> Maybe better to check that $SRANDOM is 'non-repeatable'?
Since SRANDOM can't be set how would you prove 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?
I believe that was the result of timings during testing.  As I recall 
the other tests were more specific than a few.
>
>> +  function calc_chi2() {
>> +# Calculate Chi Squared
> (snip)
>> +       #echo
>> +  }
> Please remove several '#echo' (for debugging?).
Yes, they were 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.
Okay explicit values used in messages.
>
> --
> Jun