On Wed, Sep 16, 2015 at 12:07 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
>> On Wed, Sep 16, 2015 at 11:24:27AM -0700, Junio C Hamano wrote:
>>
>>> Jeff King <peff@xxxxxxxx> writes:
>>>
>>> > On Tue, Sep 15, 2015 at 11:19:21PM -0400, Eric Sunshine wrote:
>>> >
>>> >> > strcpy(hexbuf[stage], sha1_to_hex(ce->sha1));
>>> >> > - sprintf(ownbuf[stage], "%o", ce->ce_mode);
>>> >> > + xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o", ce->ce_mode);
>>> >>
>>> >> Interesting. I wonder if there are any (old/broken) compilers which
>>> >> would barf on this. If we care, perhaps sizeof(ownbuf[0]) instead?
>>> >
>>> > Good point. I've changed it to sizeof(ownbuf[0]).
>>>
>>> Panda brain is lost here. What's the difference, other than that we
>>> will now appear to be measuring the size of the thing at index 0
>>> while using that size to stuff data into a different location? All
>>> elements of the array are of the same size so there wouldn't be any
>>> difference either way, no?
>>
>> Correct. The original is sane and gcc does the right thing. The question
>> is whether some compiler would complain that "stage" is not a constant
>> in the sizeof() expression. I don't know if any compiler would do so,
>> but it is easy enough to be conservative.
>
> Wouldn't such a compiler also complain if you did this, though?
>
> int *pointer_to_int;
> size_t sz = sizeof(*pointer_to_int);
>
> You (as a complier) do not know exactly where ownbuf[stage] is,
> because "stage" is unknown to you. In the same way, you do not know
> exactly where *pointer_to_int is. But of course, what the sizeof()
> operator is being asked is the size of the thing, which does not
> depend on where the thing is. If you (as a compiler) does not know
> that and complain to ownbuf[stage], wouldn't you complain to the
> pointer dereference, too?
>
> A more important reason I am reluctant to see this:
>
> xsnprintf(ownbuf[stage], sizeof(ownbuf[0]), "%o", ...);
>
> is that it looks strange in the same way as this
>
> memcpy(ownbuf[stage], src, sizeof(ownbuf[0]));
>
> looks strange. "We use the size of one thing to stuff into another".
>
> That will make future readers wonder "Is this a typo, and if it is,
> which index is a mistake I can fix?" and may lead to an unnecessary
> confusion. I do not want to see a correctly written
>
> xsnprintf(ownbuf[stage], sizeof(ownbuf[0]), "%o", ...);
>
> turned into
>
> xsnprintf(ownbuf[0], sizeof(ownbuf[0]), "%o", ...);
>
> out of such a confusion.
So we could just not use the bracket notation, but the pointer then?
xsnprintf(ownbuf[stage], sizeof(*ownbuf), "%o", ...);
IMHO that would reasonably well tell you that we just care about the
size of one element there.
A funny thought:
xsnprintf(ownbuf[stage], sizeof(ownbuf[-1]), "%o", ...);
should work as well as any reader would question the sanity
of a negative index.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html