Re: [PATCH 06/25] Add struct scratch_buffer and its internal helper functions

[Paul Eggert <eggert at cs dot ucla dot edu>]

On 03/01/2015 06:28 AM, Florian Weimer wrote:
> +   The non-inlined functions are implemented in such a way that it is
> +   possible to change the size of the pre-allocated buffer without
> +   impacting ABI.

Why does this matter?  The ABI is not exported to users, right?  The 
comment should explain why the ABI business matters.

> +#define SCRATCH_BUFFER_ALIGNMENT \
> +  __attribute__ ((aligned (__alignof__ (union {void *p; double d;}))))

This should use __attribute__ ((aligned (alignof (max_align_t)))), at 
least on C11 platforms.

> +struct scratch_buffer {
> +  void *data;    /* Pointer to the beginning of the scratch area.  */
> +  size_t length; /* Allocated space at the data pointer, in bytes.  */
> +  char __space[1024 - sizeof (size_t)] SCRATCH_BUFFER_ALIGNMENT;
> +} SCRATCH_BUFFER_ALIGNMENT;

Why are there two SCRATCH_BUFFER_ALIGNMENTs there?  One suffices, no?

Why 1024?  And why "- sizeof (size_t)"?  A comment should say.

> +/* Grows *BUFFER by some arbitrary amount.  The buffer contents is NOT
> +   preserved.  Returns true on success, fails on allocation failure
> +   (in which case the old buffer is freed).  On success, the new
> +   buffer is slightly larger (by at least 16 bytes) than the previous
> +   size.  On failure, *BUFFER is deallocated, but remains in a
> +   free-able state.  */

Why 16?  The comment should say.

Shouldn't the caller have some say on how big to grow the buffer?  I can 
see a caller knowing how much space it'll need.  As things stand, such a 
caller needs to repeatedly grow the buffer until it's big enough, which 
is awkward.  Hmm, I see that scratch_buffer_set_array_size lets one set 
the array size to any value, but there's no variant of 
scratch_buffer_set_array_size that preserves the buffer.

On failure this function sets errno, right?  The comment should say this.

> +/* Grows *BUFFER so that it can store at least NELEM elemnts of SIZE
> +   bytes.  The buffer contents is NOT preserved.  Returns true on
> +   success, fails on allocation failure (in which case the old buffer
> +   is freed, but *BUFFER remains a free-able state).  */

Similar remark about errno.  Also, is this function allowed to shrink 
*BUFFER?  Also, is SIZE allowed to be zero?  The comment should say.

> +  size_t new_length = buffer->length * 2;

In Gnulib we originally did it this way, but nowadays we grow by a 
factor of 1.5 (new_length = old_length + old_length / 2 + 1) rather than 
by a factor of 2, as this was less jerky for large buffers. Perhaps do 
something similar here?

> +  size_t size_max_square_root = ((size_t)1) << (sizeof (size_t) * 4);
> +  /* Avoid overflow check if both values are small. */
> +  if (nelem >= size_max_square_root || size >= size_max_square_root)

"4" is too much a mystery here and should be spelled out as CHAR_BIT / 
2.  Also, this is smaller and faster when done this way:

   if ((nelem | size) >> (sizeof (size_t) * CHAR_BIT / 2) != 0)



> +      if (nelem != 0 && size > SIZE_MAX / nelem)

This is a run-time integer division.  It can be a bit faster to do the 
division at compile-time.  Gnulib does this by computing 'SIZE_MAX / 
size' in an inline function; 'size' is normally a constant (and must be 
nonzero in Gnulib, which is a reasonable restriction here too).  With 
this optimization, Gnulib doesn't need to mess with size square roots 
either, another minor performance win.