This is the mail archive of the mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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.

+  __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;

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.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]