On Thu, Feb 02, 2017 at 11:00:44AM -0700, Martin Sebor wrote:
> 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:
Certainly not for me.
> 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.
I find these harder to read and they always give me a pause, especially with >=
or
<=. I'd say that 99% of the codebase uses "obj >= 0", so we should fix the
rest and be consistent.
Marek
