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.