René Scharfe <l.s.r@xxxxxx> writes:
> Am 24.10.2016 um 19:27 schrieb Junio C Hamano:
>> Junio C Hamano <gitster@xxxxxxxxx> writes:
>>
>>>> I think it would be preferable to just fix it inline in each place.
>>>
>>> Yeah, probably.
>>>
>>> My initial reaction to this was
>>>
>>> char *sha1_to_hex(const unsigned char *sha1)
>>> {
>>> - static int bufno;
>>> + static unsigned int bufno;
>>> static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
>>> return sha1_to_hex_r(hexbuffer[3 & ++bufno], sha1);
>>>
>>> "ah, we do not even need bufno as uint; it could be ushort or even
>>> uchar". If this were a 256 element ring buffer and the index were
>>> uchar, we wouldn't even be having this discussion, and "3 &" is a
>>> way to get a fake type that is a 2-bit unsigned integer that wraps
>>> around when incremented.
>>>
>>> But being explicit, especially when we know that we can rely on the
>>> fact that the compilers are usually intelligent enough, is a good
>>> idea, I would think.
>>>
>>> Isn't size_t often wider than uint, by the way? It somehow makes me
>>> feel dirty to use it when we know we only care about the bottom two
>>> bit, especially with the explicit "bufno %= ARRAY_SIZE(hexbuffer)",
>>> but I may be simply superstitious in this case. I dunno.
>>
>> If we are doing the wrap-around ourselves, I suspect that the index
>> should stay "int" (not even unsigned), as that is supposed to be the
>> most natural and performant type on the architecture. Would it
>> still result in better code to use size_t instead?
>
> Right.
>
> Gcc emits an extra cltq instruction for x86-64 (Convert Long To Quad,
> i.e. 32-bit to 64-bit) if we wrap explicitly and still use an int in
> sha1_to_hex(). It doesn't if we use an unsigned int or size_t. It
> also doesn't for get_pathname(). I guess updating the index variable
> only after use makes the difference there.
>
> But I think we can ignore that; it's just an extra cycle. I only
> even noticed it when verifying that "% 4" is turned into "& 3"..
> Clang and icc don't add the cltq, by the way.
>
> So how about this? It gets rid of magic number 3 and works for array
> size that's not a power of two. And as a nice side effect it can't
> trigger a signed overflow anymore.
Looks good to me. Peff?
> Just adding "unsigned" looks more attractive to me for some reason.
> Perhaps I stared enough at the code to get used to the magic values
> there..
I somehow share that feeling, too.