Something that actually returns an int would fit the name better. ;)
The particular type would not matter to the callers of the helper
macro, would it?
Three of the five callers introduced in that commit cast the result to
int and the remaining two don't care, so it actually does seem to
matter for most of them, strictly speaking. When I see a nit, I can't
resist the urge to pick it, apparently.
Unfortunately, replacing intptr_t with int or casting the above
again as int will result in
builtin/fmt-merge-msg.c: In function 'record_person':
builtin/fmt-merge-msg.c:213: error: cast to pointer from integer of different size
So...
Something that actually returns an int would fit the name better. ;)
The particular type would not matter to the callers of the helper
macro, would it?
Three of the five callers introduced in that commit cast the result to
int and the remaining two don't care, so it actually does seem to
matter for most of them, strictly speaking. When I see a nit, I can't
resist the urge to pick it, apparently.
Unfortunately, replacing intptr_t with int or casting the above
again as int will result in
builtin/fmt-merge-msg.c: In function 'record_person':
builtin/fmt-merge-msg.c:213: error: cast to pointer from integer of different size
So...
Out of the frying pan, into the fire...
builtin/fmt-merge-msg.c: In function ‘record_person’:
builtin/fmt-merge-msg.c:213:34: warning: cast from pointer to
integer of different size [-Wpointer-to-int-cast]
builtin/fmt-merge-msg.c: In function ‘cmp_string_list_util_as_int’:
builtin/fmt-merge-msg.c:219:9: warning: cast from pointer to
integer of different size [-Wpointer-to-int-cast]
builtin/fmt-merge-msg.c:219:26: warning: cast from pointer to
integer of different size [-Wpointer-to-int-cast]
builtin/fmt-merge-msg.c: In function ‘add_people_count’:
builtin/fmt-merge-msg.c:229:8: warning: cast from pointer to
integer of different size [-Wpointer-to-int-cast]
builtin/fmt-merge-msg.c:231:8: warning: cast from pointer to
integer of different size [-Wpointer-to-int-cast]
builtin/fmt-merge-msg.c:235:8: warning: cast from pointer to
integer of different size [-Wpointer-to-int-cast]
I see slightly different code in pu than in next, but it produces the
same warnings on my 64-bit Linux machine.
Here's a fix against next:
-- >8 --
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]
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.
---
builtin/fmt-merge-msg.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Jeff King <hidden> Date: 2016-06-15 22:53:18
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. 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
From: Phil Hord <hidden> Date: 2016-06-15 22:53:18
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