Re: [PATCH] fmt-merge-msg: show those involved in a merged series

On Tue, Mar 13, 2012 at 5:03 PM, Jeff King [off-list ref] wrote:

On Mon, Mar 12, 2012 at 05:37:57PM -0400, Phil Hord wrote:

quoted

Subject: [PATCH] Appease compiler pedantry with an extra cast

Recently git repurposed a pointer as an integer to hold some
counter which git fancies.

Casting directly from 'pointer' to 'int' ((int)(void*)&x) causes a
possible size mismatch because pointers can be bigger than ints.
In such a situation, the compiler complains:

   warning: cast from pointer to integer of different size
            [-Wpointer-to-int-cast]

Yeah, I've been seeing the same warning on my x86_64 box, and came up
with the same fix. However...

quoted

Cast the value through intptr_t first to quell compiler complaints
about how this gun appears to be aimed near our feet.  Then cast this
value to an int; this path assures the compiler we are smarter than we
look, or at least that we intend to aim the gun this way for a reason.

This feels so hacky.

Well, that's because it is hacky.  But it's the original code that's suspect.

One of the callsites does:

   elem->util = (void*)((intptr_t)(util_as_int(elem) + 1));

which will truncate the value down to an int before replacing it back in
the void pointer. And that truncation is ultimately what the compiler is
warning about, and what we are sneaking around with the extra cast
(because casting between integer sizes of different types is OK, even
though it can cause truncation).

I think this one is ok because it's really just "the hacky" bit
storing an integer in a variable meant to hold a pointer.  That's why
it's incrementing here, I suppose, not because it really wants to
point at the next byte.

I don't think the truncation is a problem in practice, but it just feels
like we are not just silencing an over-zealous compiler, but actually
burying type-size assumption behind a set of four (4!) casts.

The compiler is doing its job here to warn us against storing big-ish
pointers in small-ish ints.  But if we know we will never accidentally
use this as a pointer and if the integer will never overflow the
32-bounds of the (int) representation, then it's all good.

But I agree, it is hacky.

Phil