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

[Stefan Liebler <stli at linux dot vnet dot ibm dot com>]

Hi Florian,

i get a build error on s390x:
gcc scratch_buffer_grow_preserve.c -c
...
scratch_buffer_grow_preserve.c: In function 
â__libc_scratch_buffer_grow_preserveâ:
scratch_buffer_grow_preserve.c:35:7: error: implicit declaration of 
function âmemcpyâ [-Werror=implicit-function-declaration]
       memcpy (new_ptr, buffer->__space, buffer->length);
       ^
scratch_buffer_grow_preserve.c:35:7: error: incompatible implicit 
declaration of built-in function âmemcpyâ [-Werror]
cc1: all warnings being treated as errors

#include <string.h> in malloc/scratch_buffer_grow_preserve.c solves this 
error. Please update scratch_buffer_grow_preserve.c.

Bye
Stefan

On 04/07/2015 11:10 AM, Florian Weimer wrote:
> On 04/06/2015 11:54 PM, Paul Eggert wrote:
> > Thanks, this looks good.  One performance nit that I noticed this time
> > around.  here:
> >
> > On 04/06/2015 10:01 AM, Florian Weimer wrote:
> > > +  /* Avoid overflow check if both values are small. */
> > > +  if ((nelem | size) >> (sizeof (size_t) * CHAR_BIT / 2) != 0)
> > > +    {
> > > +      if (nelem != 0 && size > SIZE_MAX / nelem)
> > > +    {
> > > +      /* Overflow.  Discard the old buffer, but it must remain
> > > +         valid to free.  */
> > > +      scratch_buffer_free (buffer);
> > > +      scratch_buffer_init (buffer);
> > > +      __set_errno (ENOMEM);
> > > +      return false;
> > > +    }
> > > +    }
> > > +
> > > +  size_t new_length = nelem * size;
> >
> > It's better to compute new_length first, before the overflow check, and
> > then replace "size > SIZE_MAX / nelem" with "size != new_length /
> > nelem".  This will avoid the need for having SIZE_MAX in the executable,
> > which should shrink the code a tad.  Also, hardware integer division
> > tends to run faster with smaller dividends, which would be the case if
> > this change were made.
>
> Thanks for the suggestion.  I incorporated it into the version I
> committed after verifying it does indeed reduce code size (also attached).