[C/C++ PATCH] RFC: Implement -Wduplicated-cond (PR c/64249) (take

Martin Sebor msebor@gmail.com
Fri Sep 18 16:50:00 GMT 2015


> 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).

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).

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.

>
> 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.

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.

Martin



More information about the Gcc-patches mailing list