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

From: Phil Hord <phil.hord@gmail.com>
To: Jeff King <peff@peff.net>
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"René Scharfe" <rene.scharfe@lsrfire.ath.cx>,
	"Linus Torvalds" <torvalds@linux-foundation.org>,
	git@vger.kernel.org
Subject: Re: [PATCH] fmt-merge-msg: show those involved in a merged series
Date: Wed, 14 Mar 2012 15:12:08 -0400	[thread overview]
Message-ID: <CABURp0qE9XRg+a8WRdicojJNqcLnZn_PozFTDO8qZ8BMM1NsJQ@mail.gmail.com> (raw)
In-Reply-To: <20120313210357.GC27436@sigill.intra.peff.net>

On Tue, Mar 13, 2012 at 5:03 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Mar 12, 2012 at 05:37:57PM -0400, Phil Hord wrote:
>
>> 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.

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