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.
-Peff