[PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jeff Law]

On 02/14/2017 01:32 PM, Jakub Jelinek wrote:
> On Tue, Feb 14, 2017 at 12:15:59PM -0700, Martin Sebor wrote:
>> That comment explains how the likely_adjust variable ("the adjustment")
>> is being used, or more precisely, how it was being used in the first
>> version of the patch.  The comment became somewhat out of date with
>> the committed version of the patch (this was my bad).
>>
>> The variable is documented where it's defined and again where it's
>> assigned to.  With the removal of those comments it seems especially
>> important that the only remaining description of what's going on be
>> accurate.
>>
>> The comment is outdated because it refers to "the adjustment" which
>> doesn't exist anymore.  (It was replaced by a flag in my commit).
>> To bring it up to date it should say something like:
>>
>>   /* Set the LIKELY counter to MIN.  In base 8 and 16, when
>>      the argument is in range that includes zero, adjust it
>>      upward to include the length of the base prefix since
>>      in that case the MIN counter does include it.  */
>
> So for a comment, what about following then?  With or without
> the IMNSHO useless
> && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)
>
>> On a separate note, while testing the patch I noticed that it's
>> not exactly equivalent to what's on trunk.  Trunk silently accepts
>> the call below but with the patch it complains.  That's great (it
>> should complain) but the change should be tested.  More to my point,
>> while in this case your change happened to fix a subtle bug (which
>> I'm certainly happy about), it could have just as easily introduced
>> one.
>
> Yeah, indeed.  That should be a clear argument for why writing it in
> so many places is bad, it is simply much more error-prone, there are
> too many cases to get right.
>
>>   char d[2];
>>
>>   void f (unsigned i)
>>   {
>>     if (i < 1234 || 12345 < i)
>>       i = 1234;
>>
>>     __builtin_sprintf (d, "%#hhx", i);
>>   }
>
> What happens is that because the original range doesn't contain zero
> you set likely_adjust to false and then never update it again because
> the implicit cast changed the range.
>
> If some version of the patch is approved, I'll leave addition of this
> testcase to you (incrementally).
>
> 2017-02-14  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR tree-optimization/79327
> 	* gimple-ssa-sprintf.c (format_integer): Remove likely_adjust
> 	variable, its initialization and use.
This is fine.  And the addition of the test from Martin is pre-approved 
as well.

jeff