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

[Marek Polacek <polacek at redhat dot com>]

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