Re: [PATCH 3/4] cache.h: hex2chr() - avoid -Wsign-compare warnings

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

On 22/09/17 17:11, Jeff King wrote:
> On Fri, Sep 22, 2017 at 05:05:03PM +0100, Ramsay Jones wrote:
> 
>>> As an aside, I also see some uses of hexval() that don't appear to be
>>> quite as rigorous in checking for invalid characters. A few
>>> unconditionally shift the first nibble and assume that there will still
>>> be high bits set. I think that's generally true for twos-complement
>>> negative numbers, but isn't shifting off the left side of a signed
>>> integer undefined behavior?
>>
>> All uses of hexval() that I can see are shifting an unsigned value.
>> Have I missed something?
> 
> Hmm. get_hex_color() does:
> 
>   unsigned int val;
>   val = (hexval(in[0]) << 4) | hexval(in[1]));
> 
> Isn't that shifting the signed return value of hexval(), and then
> converting it to unsigned afterwards?

Err ... no. the return value of hexval() is *unsigned int*.
(which is kinda the point!)

> I've been confused by C's integer conversion rules before, though, so
> perhaps I'm wrong.
> 
> I think if this function is fed an empty string that it will also read
> past the end of the buffer for in[1]. It shouldn't matter, since the NUL
> in in[0] would cause us to return an error regardless, but it's still
> undefined behavior.

Correct.

> In fact, this whole function is just hex2chr() implemented badly. Who is
> responsible for this terrible code? ;)

;-)

ATB,
Ramsay Jones