On 02/14/2017 09:39 AM, Jakub Jelinek wrote:
On Tue, Feb 14, 2017 at 09:36:44AM -0700, Martin Sebor wrote:
@@ -1371,7 +1354,8 @@ format_integer (const directive &dir, tr
else
{
res.range.likely = res.range.min;
- if (likely_adjust && maybebase && base != 10)
+ if (maybebase && base != 10
+ && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0))
{
if (res.range.min == 1)
res.range.likely += base == 8 ? 1 : 2;
You've removed all the comments that explain what's going on. If
you must make the change (I see no justification for it now) please
at least document it.
I thought that is the:
/* Add the adjustment for an argument whose range includes zero
since it doesn't include the octal or hexadecimal base prefix. */
comment above the if.
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. */
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.
char d[2];
void f (unsigned i)
{
if (i < 1234 || 12345 < i)
i = 1234;
__builtin_sprintf (d, "%#hhx", i);
}
Martin
