Re: [PATCH] pack-bitmap: do not core dump

From: Kyle J. McKay <mackyle@gmail.com>
Date: Apr 23, 2014, 00:40 UTC
Message-ID: <CC07C833-D42A-488D-8523-46445F8C1C20@gmail.com>
In-Reply-To: <20140422231748.GA20353@sigill.intra.peff.net>

On Apr 22, 2014, at 16:17, Jeff King wrote:

> but I do not think that is necessarily any more readable, especially
> because we probably need to cast it like:
>
>  self->rlw = (eword_t *)((uint8_t *)self->buffer + rlw_offset);

I suspect that will produce a warning about a cast increasing pointer alignment in some cases.

> So why do any uint8_t math in the first place?

Just guessing it started out as uint8_t math throughout then the warning about increasing alignment showed up so that part got changed but the save pointer offset part did not.

Show 5 quoted lines

> I think we could write it as:
>
> 	eword_t *old = self->buffer;
> 	... realloc ...
> 	self->rlw = self->buffer + (self->rlw - old);

Yeah that seems good too.

> I'm fine with your patch, though.

thanks.

I tend to gravitate towards the most minimal change that fixes the issue when touching someone else's code especially if I'm not familiar with it. There's also the minor issue of optimizing out the pointer arithmetic implicit divide by sizeof(eword_t) for the subtract and the implicit multiply by sizeof(eword_t) for the add -- I didn't check to see if one of the alternatives is best -- the one you mention above with the cast (keeping the uint8_t math) is probably guaranteed not to have any implicit multiply, but there's that pesky warning to get rid of. Of course those multiplies and divides should just end up being shifts so not a big deal if they didn't get optimized out (the realloc time should dwarf them into irrelevancy anyway).

--Kyle