On Sat, Oct 15, 2016 at 06:23:11PM +0200, René Scharfe wrote:

> Calculating offsets involving a NULL pointer is undefined.  It works in
> practice (for now?), but we should not rely on it.  Allocate first and
> then simply refer to the flexible array member by its name instead of
> performing pointer arithmetic up front.  The resulting code is slightly
> shorter, easier to read and doesn't rely on undefined behaviour.

Yeah, this NULL computation is pretty nasty. I recall trying to get rid
of it, but I think it is impossible to do so portably while still using
the generic xalloc_flex() helper. I'm not sure why I didn't think of
just inlining it as you do here. It's not that many lines of code, and I
I agree the result is conceptually simpler.

>  #define FLEX_ALLOC_MEM(x, flexname, buf, len) do { \
> -     (x) = NULL; /* silence -Wuninitialized for offset calculation */ \
> -     (x) = xalloc_flex(sizeof(*(x)), (char *)(&((x)->flexname)) - (char 
> *)(x), (buf), (len)); \
> +     size_t flex_array_len_ = (len); \
> +     (x) = xcalloc(1, st_add3(sizeof(*(x)), flex_array_len_, 1)); \
> +     memcpy((void *)(x)->flexname, (buf), flex_array_len_); \

This looks correct. I wondered at first why you bothered with
flex_array_len, but it is to avoid evaluating the "len" parameter
multiple times.

>  } while (0)
>  #define FLEXPTR_ALLOC_MEM(x, ptrname, buf, len) do { \
>       (x) = xalloc_flex(sizeof(*(x)), sizeof(*(x)), (buf), (len)); \

Now that xalloc_flex() has only this one caller remaining, perhaps it
should just be inlined here, too, for simplicity.


Reply via email to