{"thread":{"id":"46802","subject":"[PATCH 3/4] cache.h: hex2chr() - avoid -Wsign-compare warnings","startedAt":"2017-09-21T16:48:44Z","lastAt":"2017-09-22T18:01:59Z","messageCount":8,"participants":["Ramsay Jones","Jeff King","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"328577","messageId":"ae537e7e-2c3d-f7f3-28f4-451c37c01bbb@ramsayjones.plus.com","threadId":"46802","inReplyTo":null,"subject":"[PATCH 3/4] cache.h: hex2chr() - avoid -Wsign-compare warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-09-21T16:48:38Z","receivedAt":"2017-09-21T16:48:44Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n cache.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex a916bc79e..a0e3e362c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1243,8 +1243,8 @@ static inline unsigned int hexval(unsigned char c)\n  */\n static inline int hex2chr(const char *s)\n {\n-\tint val = hexval(s[0]);\n-\treturn (val < 0) ? val : (val << 4) | hexval(s[1]);\n+\tunsigned int val = hexval(s[0]);\n+\treturn (val & ~0xf) ? val : (val << 4) | hexval(s[1]);\n }\n \n /* Convert to/from hex/sha1 representation */\n-- \n2.14.0\n"},{"id":"328623","messageId":"20170922054748.iseinawwwfw56vis@sigill.intra.peff.net","threadId":"46802","inReplyTo":"ae537e7e-2c3d-f7f3-28f4-451c37c01bbb@ramsayjones.plus.com","subject":"Re: [PATCH 3/4] cache.h: hex2chr() - avoid -Wsign-compare warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-22T05:47:48Z","receivedAt":"2017-09-22T05:47:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 21, 2017 at 05:48:38PM +0100, Ramsay Jones wrote:\n\n> diff --git a/cache.h b/cache.h\n> index a916bc79e..a0e3e362c 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1243,8 +1243,8 @@ static inline unsigned int hexval(unsigned char c)\n>   */\n>  static inline int hex2chr(const char *s)\n>  {\n> -\tint val = hexval(s[0]);\n> -\treturn (val < 0) ? val : (val << 4) | hexval(s[1]);\n> +\tunsigned int val = hexval(s[0]);\n> +\treturn (val & ~0xf) ? val : (val << 4) | hexval(s[1]);\n>  }\n\nIronically, the unsigned return from hexval() comes from internally\nconverting the signed char in hexval_table. And then we again return it\nas a signed int from hex2chr().\n\nWould it make sense to return a signed int from hexval()? That would\nmake hex2chr just work as it tries to above. I admit that shifting\nsigned values is a little funny, but it should be fine here since we\nknow they're no larger than 8 bits in the first place.\n\nAs an aside, I also see some uses of hexval() that don't appear to be\nquite as rigorous in checking for invalid characters. A few\nunconditionally shift the first nibble and assume that there will still\nbe high bits set. I think that's generally true for twos-complement\nnegative numbers, but isn't shifting off the left side of a signed\ninteger undefined behavior?\n\nAnd mailinfo's decode_q_segment() does not seem to check for errors at\nall.\n\nHandling those is getting far off your original patch, but I'm having\ntrouble figuring out if it's saner for us to consistently stick to\nall-signed or all-unsigned here.\n\n-Peff\n"},{"id":"328641","messageId":"89beb424-2cc0-bfe3-7847-c11035bfec70@ramsayjones.plus.com","threadId":"46802","inReplyTo":"20170922054748.iseinawwwfw56vis@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] cache.h: hex2chr() - avoid -Wsign-compare warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-09-22T16:05:03Z","receivedAt":"2017-09-22T16:05:09Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 22/09/17 06:47, Jeff King wrote:\n> On Thu, Sep 21, 2017 at 05:48:38PM +0100, Ramsay Jones wrote:\n> \n>> diff --git a/cache.h b/cache.h\n>> index a916bc79e..a0e3e362c 100644\n>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -1243,8 +1243,8 @@ static inline unsigned int hexval(unsigned char c)\n>>   */\n>>  static inline int hex2chr(const char *s)\n>>  {\n>> -\tint val = hexval(s[0]);\n>> -\treturn (val < 0) ? val : (val << 4) | hexval(s[1]);\n>> +\tunsigned int val = hexval(s[0]);\n>> +\treturn (val & ~0xf) ? val : (val << 4) | hexval(s[1]);\n>>  }\n> \n> Ironically, the unsigned return from hexval() comes from internally\n> converting the signed char in hexval_table. And then we again return it\n> as a signed int from hex2chr().\n\nYep, my first inclination was to change the return type of\nhexval(), but after looking at all its callers, I decided\nagainst that (because it wouldn't help). ;-)\n\n> Would it make sense to return a signed int from hexval()? That would\n> make hex2chr just work as it tries to above. I admit that shifting\n> signed values is a little funny, but it should be fine here since we\n> know they're no larger than 8 bits in the first place.\n\nIndeed, shifting signed values is a no-no, which is why I decided\nto go this route.\n\n> As an aside, I also see some uses of hexval() that don't appear to be\n> quite as rigorous in checking for invalid characters. A few\n> unconditionally shift the first nibble and assume that there will still\n> be high bits set. I think that's generally true for twos-complement\n> negative numbers, but isn't shifting off the left side of a signed\n> integer undefined behavior?\n\nAll uses of hexval() that I can see are shifting an unsigned value.\nHave I missed something?\n> And mailinfo's decode_q_segment() does not seem to check for errors at\n> all.\n\nYes, I noticed that. (I put it on my TODO list).\n\n> Handling those is getting far off your original patch, but I'm having\n> trouble figuring out if it's saner for us to consistently stick to\n> all-signed or all-unsigned here.\n\nOh, unsigned, without a doubt. :D\n\nATB,\nRamsay Jones\n\n\n"},{"id":"328642","messageId":"20170922161159.hkw4id4kpnwo7jou@sigill.intra.peff.net","threadId":"46802","inReplyTo":"89beb424-2cc0-bfe3-7847-c11035bfec70@ramsayjones.plus.com","subject":"Re: [PATCH 3/4] cache.h: hex2chr() - avoid -Wsign-compare warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-22T16:11:59Z","receivedAt":"2017-09-22T16:12:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 22, 2017 at 05:05:03PM +0100, Ramsay Jones wrote:\n\n> > As an aside, I also see some uses of hexval() that don't appear to be\n> > quite as rigorous in checking for invalid characters. A few\n> > unconditionally shift the first nibble and assume that there will still\n> > be high bits set. I think that's generally true for twos-complement\n> > negative numbers, but isn't shifting off the left side of a signed\n> > integer undefined behavior?\n> \n> All uses of hexval() that I can see are shifting an unsigned value.\n> Have I missed something?\n\nHmm. get_hex_color() does:\n\n  unsigned int val;\n  val = (hexval(in[0]) << 4) | hexval(in[1]));\n\nIsn't that shifting the signed return value of hexval(), and then\nconverting it to unsigned afterwards?\n\nI've been confused by C's integer conversion rules before, though, so\nperhaps I'm wrong.\n\nI think if this function is fed an empty string that it will also read\npast the end of the buffer for in[1]. It shouldn't matter, since the NUL\nin in[0] would cause us to return an error regardless, but it's still\nundefined behavior.\n\nIn fact, this whole function is just hex2chr() implemented badly. Who is\nresponsible for this terrible code? ;)\n\n-Peff\n"},{"id":"328643","messageId":"20170922161817.iy5tpexouxn5uwsb@sigill.intra.peff.net","threadId":"46802","inReplyTo":"20170922161159.hkw4id4kpnwo7jou@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] cache.h: hex2chr() - avoid -Wsign-compare warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-22T16:18:17Z","receivedAt":"2017-09-22T16:18:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 22, 2017 at 12:11:59PM -0400, Jeff King wrote:\n\n> On Fri, Sep 22, 2017 at 05:05:03PM +0100, Ramsay Jones wrote:\n> \n> > > As an aside, I also see some uses of hexval() that don't appear to be\n> > > quite as rigorous in checking for invalid characters. A few\n> > > unconditionally shift the first nibble and assume that there will still\n> > > be high bits set. I think that's generally true for twos-complement\n> > > negative numbers, but isn't shifting off the left side of a signed\n> > > integer undefined behavior?\n> > \n> > All uses of hexval() that I can see are shifting an unsigned value.\n> > Have I missed something?\n> \n> Hmm. get_hex_color() does:\n> \n>   unsigned int val;\n>   val = (hexval(in[0]) << 4) | hexval(in[1]));\n> \n> Isn't that shifting the signed return value of hexval(), and then\n> converting it to unsigned afterwards?\n> \n> I've been confused by C's integer conversion rules before, though, so\n> perhaps I'm wrong.\n\nOh, nevermind. I managed to confuse myself again. The return value from\nhexval() is already unsigned. It's hex2chr that has the funny signed\nreturn. So the signedness is fine.\n\n> I think if this function is fed an empty string that it will also read\n> past the end of the buffer for in[1]. It shouldn't matter, since the NUL\n> in in[0] would cause us to return an error regardless, but it's still\n> undefined behavior.\n\nThis is still a bug, though.\n\n-Peff\n"},{"id":"328644","messageId":"20170922162251.kx2w5u4falztcpnh@sigill.intra.peff.net","threadId":"46802","inReplyTo":"20170922161817.iy5tpexouxn5uwsb@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] cache.h: hex2chr() - avoid -Wsign-compare warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-22T16:22:51Z","receivedAt":"2017-09-22T16:22:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 22, 2017 at 12:18:17PM -0400, Jeff King wrote:\n\n> > I think if this function is fed an empty string that it will also read\n> > past the end of the buffer for in[1]. It shouldn't matter, since the NUL\n> > in in[0] would cause us to return an error regardless, but it's still\n> > undefined behavior.\n> \n> This is still a bug, though.\n\nLast message, I promise. ;)\n\nI started on the minimal fix for this, but actually it's OK by virtue of\nits sole caller first checking that we have enough length (because we're\nnot parsing a string, in fact, but a ptr/len buffer).\n\nSo all is well, though I think get_hex_color() does serve as a poor\nexample if somebody were to try to adapt it generally (hopefully they\nwouldn't, since hex2chr() is already globally available).\n\nSorry for the all the noise.\n\n-Peff\n"},{"id":"328645","messageId":"d47cdae8-b37f-f378-6df6-bf70a819dd5b@ramsayjones.plus.com","threadId":"46802","inReplyTo":"20170922161159.hkw4id4kpnwo7jou@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] cache.h: hex2chr() - avoid -Wsign-compare warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-09-22T16:24:00Z","receivedAt":"2017-09-22T16:24:19Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 22/09/17 17:11, Jeff King wrote:\n> On Fri, Sep 22, 2017 at 05:05:03PM +0100, Ramsay Jones wrote:\n> \n>>> As an aside, I also see some uses of hexval() that don't appear to be\n>>> quite as rigorous in checking for invalid characters. A few\n>>> unconditionally shift the first nibble and assume that there will still\n>>> be high bits set. I think that's generally true for twos-complement\n>>> negative numbers, but isn't shifting off the left side of a signed\n>>> integer undefined behavior?\n>>\n>> All uses of hexval() that I can see are shifting an unsigned value.\n>> Have I missed something?\n> \n> Hmm. get_hex_color() does:\n> \n>   unsigned int val;\n>   val = (hexval(in[0]) << 4) | hexval(in[1]));\n> \n> Isn't that shifting the signed return value of hexval(), and then\n> converting it to unsigned afterwards?\n\nErr ... no. the return value of hexval() is *unsigned int*.\n(which is kinda the point!)\n\n> I've been confused by C's integer conversion rules before, though, so\n> perhaps I'm wrong.\n> \n> I think if this function is fed an empty string that it will also read\n> past the end of the buffer for in[1]. It shouldn't matter, since the NUL\n> in in[0] would cause us to return an error regardless, but it's still\n> undefined behavior.\n\nCorrect.\n\n> In fact, this whole function is just hex2chr() implemented badly. Who is\n> responsible for this terrible code? ;)\n\n;-)\n\nATB,\nRamsay Jones\n\n\n"},{"id":"328662","messageId":"5e21362b-7c42-37f5-73a8-907d4ad5db86@web.de","threadId":"46802","inReplyTo":"20170922054748.iseinawwwfw56vis@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] cache.h: hex2chr() - avoid -Wsign-compare warnings","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-09-22T18:01:26Z","receivedAt":"2017-09-22T18:01:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 22.09.2017 um 07:47 schrieb Jeff King:\n> On Thu, Sep 21, 2017 at 05:48:38PM +0100, Ramsay Jones wrote:\n> \n>> diff --git a/cache.h b/cache.h\n>> index a916bc79e..a0e3e362c 100644\n>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -1243,8 +1243,8 @@ static inline unsigned int hexval(unsigned char c)\n>>    */\n>>   static inline int hex2chr(const char *s)\n>>   {\n>> -\tint val = hexval(s[0]);\n>> -\treturn (val < 0) ? val : (val << 4) | hexval(s[1]);\n>> +\tunsigned int val = hexval(s[0]);\n>> +\treturn (val & ~0xf) ? val : (val << 4) | hexval(s[1]);\n>>   }\n> \n> Ironically, the unsigned return from hexval() comes from internally\n> converting the signed char in hexval_table. And then we again return it\n> as a signed int from hex2chr().\n> \n> Would it make sense to return a signed int from hexval()? That would\n> make hex2chr just work as it tries to above. I admit that shifting\n> signed values is a little funny, but it should be fine here since we\n> know they're no larger than 8 bits in the first place.\n\nI'd say yes.  We can replace that shift with a multiplication --\ncompilers will nevertheless use a shift even with -O0 (if\nbeneficial).\n\n> As an aside, I also see some uses of hexval() that don't appear to be\n> quite as rigorous in checking for invalid characters. A few\n> unconditionally shift the first nibble and assume that there will still\n> be high bits set. I think that's generally true for twos-complement\n> negative numbers, but isn't shifting off the left side of a signed\n> integer undefined behavior?\n> \n> And mailinfo's decode_q_segment() does not seem to check for errors at\n> all.\n\nIt also allocates and returns a strbuf, which seems odd as well.\n\nRené\n"}]}