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

From: Jeff King <peff@peff.net>
Date: Mar 13, 2012, 21:03 UTC
Message-ID: <20120313210357.GC27436@sigill.intra.peff.net>
In-Reply-To: <CABURp0oVn3s27Rtq2JQeLBgcXq1-R-=0qawcXVMxZ8qgOeZGkQ@mail.gmail.com>

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

Show 11 quoted lines

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

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

I wonder if we would be happier to declare the "util" field of string_list as a union. Obviously that provides no safety that we read the correct item out of the union, but that is no worse than the situation with all of these casts, and I suspect the result would be much more obvious and readable.

The downside is that all current users of the "util" field would need s/util/&.ptr/ or similar to dereference the pointer field.

-Peff