Re: [PATCH] relax -Wformat-overflow for precision ranges (PR 79275)

[Martin Sebor <msebor at gmail dot com>]

On 02/02/2017 10:26 AM, Jeff Law wrote:
> On 01/30/2017 02:28 PM, Martin Sebor wrote:
> > Bug 79275 - -Wformat-overflow false positive exceeding INT_MAX in
> > glibc sysdeps/posix/tempname.c points out a false positive found
> > during a Glibc build and caused by the checker using the upper
> > bound of a range of precisions in string directives with string
> > arguments of non-constant length.  The attached patch relaxes
> > the checker to use the lower bound instead when appropriate.
> >
> > Martin
> >
> > gcc-79275.diff
> >
> >
> > PR middle-end/79275 -  -Wformat-overflow false positive exceeding
> > INT_MAX in glibc sysdeps/posix/tempname.c
> >
> > gcc/testsuite/ChangeLog:
> >
> >     PR middle-end/79275
> >     * gcc.dg/tree-ssa/builtin-sprintf-warn-11.c: New test.
> >     * gcc.dg/tree-ssa/pr79275.c: New test.
> >
> > gcc/ChangeLog:
> >
> >     PR middle-end/79275
> >     * gimple-ssa-sprintf.c (get_string_length): Set lower bound to zero.
> >     (format_string): Tighten up the range of output for non-constant
> >     strings and correct the expected range for wide non-constant strings.
> Couple more nits.
>
> First, I expect the patch won't apply as-is with the operand order
> fixes.  There'll be trivial changes you'll need to make for that.

I had to back out those changes to avoid the massive conflict when
applying this patch and before retesting it.  Then I reapplied those
changes by hand.

> Along the same lines, this patch would introduce a new operand order nit
> here:
>
>
> > +    }
> > +      else if (0 <= dir.prec[1])
> > +    {

In light of the hassle this has caused (above, plus with the big
patch earlier) I can't help but question the value in making these
changes.  Isn't the code just as easy to read this way as the other?

          else if (dir.prec[1] >= 0)

Wouldn't it be if the 0 were spelled ZERO?

I never even thought about it until you started pointing it out,
but my own personal (subconscious) bias is clear from how I write
the code.  I'm also not the only one.

FWIW, the example above may not be one of them but there are cases
when the constant on the left actually makes the code much easier
to read than the other way, such as:

  gcc/read-rtl-function.c:      if (0 == strncmp (desc, "orig:", 5))

or

  cp/decl.c:            if (1 == simple_cst_equal (TREE_PURPOSE (t1)

It seems to me that we should be able to write these expressions
the way that's natural to us and at the same time be able to
comfortably read them both ways.  As always, I fully support
consistency and following a coding style where it matters.  I
just don't think this does.

> Please consider documenting how we handle strings with unknown lengths.
>
>
> I don't think those warrant waiting for another review round.  Fix,
> bootstrap, test and install.

Will do.

Thanks
Martin