Re: [PATCH] Universally-unique gensyms

[Ludovic Courtès]

Hi Mark,

Mark H Weaver <address@hidden> 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 <address@hidden>
> 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’.