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

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

On 04/02/2015 11:30 AM, Florian Weimer wrote:
> +static inline void
> +scratch_buffer_init (struct scratch_buffer *buffer)
> +  __attribute__ ((always_inline));
> +static inline void

This should be just "static __always_inline void"; there should be no 
need to have both decl and defn, and __always_inline arranges for both 
'inline' and the attribute.

But then again, why bother to declare this function to be inline? Isn't 
the compiler smart enough to inline when needed?  I assume that this 
include file is not visible to the user, so is there some reason to 
require the function to be __always_inline, or even __inline?

Similarly for the other functions in this header -- why bother to 
declare them to be inline?  Can't the compiler figure it out?  Why is it 
relevant to the ABI whether the functions are inlined, if the header is 
not user-visible?  These functions are all private to glibc, right?
> +  /* Discard old buffer.  */
> +  scratch_buffer_free (buffer);
> +
> +  /* Check for overflow.  */
> +  if (__glibc_unlikely (new_length < buffer->length))
> +    {
> +      /* Buffer must remain valid to free.  */
> +      scratch_buffer_init (buffer);
> +      __set_errno (ENOMEM);
> +      return false;
> +    }
> +
> +  void *new_ptr = malloc (new_length);
> +  if (new_ptr == NULL)
> +    {
> +      /* Buffer must remain valid to free.  */
> +      scratch_buffer_init (buffer);
> +      return false;
> +    }

The two ifs can be combined so that there's only copy of the cleanup 
code.  Also, if all that's needed is that the buffer remains valid to 
free, why not let the caller's free (which we need anyway) do the 
cleanup rather than doing the cleanup redundantly ourselves? Instead of 
the above 19 lines, we could have just 6 lines:

 void *new_ptr = new_length < buffer->length ? NULL : malloc (new_length);
 if (__glibc_unlikely (! new_ptr))
   {
      __set_errno (ENOMEM);
      return false;
   }

Similarly for __libc_scratch_buffer_grow_preserve and for 
__libc_scratch_buffer_set_array_size.  The latter fix will fix the 
problem that when __libc_scratch_buffer_grow_preserve fails, sometimes 
it reinitializes the buffer to be empty and sometimes it doesn't, which 
is inconsistent.

> +	 buffer->length describes a small buffer on the stack.  We use
> +	 buffer->length instead of sizeof (buffer->__space) to avoid
> +	 making the size of struct scratch_buffer part of the ABI. */

Sorry, I still don't get why the ABI matters here, since the functions 
are all private and presumably are all linked into glibc already.