On Fri, Sep 18, 2015 at 10:45:33AM -0600, Martin Sebor wrote:
> >Done in the below. This version actually bootstraps, because I've added
> >-Wno-duplicated-cond for insn-dfatab.o and insn-latencytab.o (don't know
> >how to fix these) + I've tweaked a condition in genemit.c. The problem
> >here is that we have
> >
> > if (INTVAL (x) == 0)
> > printf ("const0_rtx");
> > else if (INTVAL (x) == 1)
> > printf ("const1_rtx");
> > else if (INTVAL (x) == -1)
> > printf ("constm1_rtx");
> > // ...
> > else if (INTVAL (x) == STORE_FLAG_VALUE)
> > printf ("const_true_rtx");
> >
> >and STORE_FLAG_VALUE happens to be 1, so we have two same conditions.
> >STORE_FLAG_VALUE is 1 or -1, but according to the documentation it can
> >also be some other number so we should keep this if statement. I've
> >avoided the warning by adding STORE_FLAG_VALUE > 1 check.
>
> Binutils and GLIBC also fail to build due to similar problems (in
> addition to several errors triggered by the new -Wunused-const-variable
> warning).
Thanks for doing this. I've been meaning to compile a kernel with this
new warning on but I've never gotten to do it.
> The one in GLIBC is trivial to fix by guarding the code with
> #if N != 1:
>
> In file included from ../sysdeps/x86_64/ldbl2mpn.c:1:0:
> ../sysdeps/x86_64/../i386/ldbl2mpn.c: In function
> ‘__mpn_extract_long_double’:
> ../sysdeps/x86_64/../i386/ldbl2mpn.c:89:24: error: duplicated ‘if’ condition
> [-Werror=duplicated-cond]
> else if (res_ptr[0] != 0)
> ^
> ../sysdeps/x86_64/../i386/ldbl2mpn.c:74:23: note: previously used here
> if (res_ptr[N - 1] != 0)
> ^
>
> The one in Binutils is pretty easy to fix too:
>
> In function ‘stab_int_type’:
> /home/msebor/scm/fsf/binutils-gdb/binutils/wrstabs.c:665:18: error:
> duplicated if’ condition [-Werror=duplicated-cond]
> else if (size == 8)
> ^
> /home/msebor/scm/fsf/binutils-gdb/binutils/wrstabs.c:663:18: note:
> previously used here
> else if (size == sizeof (long))
> ^
>
> but it makes me wonder how common this pattern is in portable
> code and whether adding workarounds for it is the right solution
> (or if it might prompt people to disable the warning, which would
> be a shame).
Yeah :(. I hate how such obscure stuff (we have *one* occurence in the
whole GCC codebase...) can paralyze the warning as such.
I'm sorry to have to say that I don't quite know what to do, because
these "false positives" aren't easily fixable. Because for
"m == sizeof (int)" the FE simply sees "m == 4"; ditto for e.g.
#define M 4 and "m == M".
Perhaps move the warning into -Wextra until we have the capacity to
differentiate between those?
> As an aside, I would have expected the change you implemented
> in GCC to get around this to trigger some other warning (such
> as -Wtautological-compare) e.g., if (a > 1 && a == -1), but it
> doesn't seem to, either in GCC or Clang.
This sort of stuff isn't really trivial to do in the front end,
I'm afraid.
> >How does this look like now?
>
> If no one else is concerned about the above it looks good to
> me. I was hoping to see the warning emitted for conditional
> expressions as well but that can be considered an enhancement.
I think this can surely be done later on.
I realized that current patch has a minor deficiency: it will start
a chain even in case the first condition has a side-effect thus the
chain should be invalid. I'll fix this problem soon.
> FWIW, while testing the patch I noticed the following bug: 67629.
> It seems like the same logic was we discussed in this context is
> needed there as well.
That sounds more like a bug in tree-cfg than in the FE ;-).
Thanks,
Marek
