Hi Mark,
Mark H Weaver <m...@netris.org> skribis:
> Here's an improved implementation, incorporating your excellent feedback
> and also implementing thread-local gensym counters, thus eliminating the
> mutex altogether. What do you think?
This looks cool, though I must confess I don’t see why sizeof (int) is
not enough for everyone, nor whether this warrants adding all this code.
A few comments.
> From 9090dfeb58846d637150f5f88e344c7d980efdf2 Mon Sep 17 00:00:00 2001
> From: Mark H Weaver <m...@netris.org>
> Date: Wed, 18 Jan 2012 02:36:17 -0500
> Subject: [PATCH 1/2] Add `random-state-from-platform' and
> `scm_i_random_bytes_from_platform'
>
> * libguile/random.c (scm_random_state_from_platform): New procedure to
> construct a new random state seeded from a platform-specific source of
> randomness.
>
> (scm_i_random_bytes_from_platform): New internal function to fill a
> byte buffer with random bytes from a platform-specific source of
> randomness.
>
> (read_dev_urandom, random_state_of_last_resort): New internal static
> helper functions.
Could you stick to GNU-style change logs, describing the change (for
example, “New function”), and not the rationale, function, etc.?
(Andy might disagree with me, but don’t listen to him. ;-))
> +static SCM
> +random_state_of_last_resort (void)
> +{
Could you a sentence above new functions describing what they return
(like you did for some of them)?
> + FILE *f = fopen ("/dev/urandom", "r");
I’ve just checked it’s available on GNU, FreeBSD, Solaris, and Darwin (I
thought it was less portable.)
> + static const char base64[GENSYM_RADIX] =
> + "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789$@";
> + static const char base4[4] = "_.-~";
Could we use Gnulib’s ‘base64’ module instead?
> + /* encode digit_buf as base64, except for the first character which is
Please capitalize sentences. :-)
> --- a/libguile/threads.h
> +++ b/libguile/threads.h
> @@ -81,6 +81,10 @@ typedef struct scm_i_thread {
> SCM dynamic_state;
> SCM dynwinds;
>
> + /* Thread-local gensym counter.
> + */
> + unsigned char *gensym_counter;
Apparently this doesn’t break the ABI, right?
Thanks,
Ludo’.
