Re: Implement -Wduplicated-branches (PR c/64279) (v3)

This patch introduced a new testsuite failure: FAIL: g++.dg/warn/Wduplicated-branches1.C -std=gnu++98 (test for excess errors) Excess errors: /nasfarm/edelsohn/src/src/gcc/testsuite/g++.dg/warn/Wduplicated-branches1.C:9:3: warning: this condition has identical branches [-Wduplicated-branches]

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On Fri, Jan 20, 2017 at 10:05:22AM -0500, David Edelsohn wrote: > This patch introduced a new testsuite failure: > > FAIL: g++.dg/warn/Wduplicated-branches1.C -std=gnu++98 (test for excess > errors) > Excess errors: >

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On Mon, Jan 16, 2017 at 03:32:59PM -0700, Jeff Law wrote: > s/indentical/identical in the doc/invoke.texi changes. Fixed. > > diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c > > index 96e7351..ed8ffe4 100644 > > --- gcc/c/c-typeck.c > > +++ gcc/c/c-typeck.c > > @@ -5193,6 +5193,15 @@

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On Thu, Jan 19, 2017 at 05:52:14PM +0100, Marek Polacek wrote: > > I agree that not warning for > > if (foo) > > return NULL; > > else > > return NULL; > > is bad. But how can I compare those expressions side-by-side? I'm not > > finding > > anything. :( > > Seems like ENOTIME to

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On Mon, Jan 09, 2017 at 02:39:30PM +0100, Marek Polacek wrote: > On Mon, Jan 09, 2017 at 12:18:01PM +0100, Jakub Jelinek wrote: > > On Mon, Jan 09, 2017 at 10:21:47AM +0100, Marek Polacek wrote: > > > +/* Callback function to determine whether an expression TP or one of its > > > +

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On 01/09/2017 02:21 AM, Marek Polacek wrote: On Thu, Jan 05, 2017 at 04:41:28PM +0100, Jakub Jelinek wrote: On Thu, Jan 05, 2017 at 04:39:40PM +0100, Marek Polacek wrote: Coming back to this... Right, after h0 == h1 is missing && operand_equal_p (thenb, elseb, 0) or so (the exact last

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On Mon, Jan 09, 2017 at 12:18:01PM +0100, Jakub Jelinek wrote: > On Mon, Jan 09, 2017 at 10:21:47AM +0100, Marek Polacek wrote: > > +/* Callback function to determine whether an expression TP or one of its > > + subexpressions comes from macro expansion. Used to suppress bogus > > + warnings.

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On Mon, Jan 09, 2017 at 10:21:47AM +0100, Marek Polacek wrote: > +/* Callback function to determine whether an expression TP or one of its > + subexpressions comes from macro expansion. Used to suppress bogus > + warnings. */ > + > +static tree > +expr_from_macro_expansion_r (tree *tp, int

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On Mon, Jan 09, 2017 at 11:57:48AM +0100, Richard Biener wrote: > On Mon, 9 Jan 2017, Marek Polacek wrote: > > > On Thu, Jan 05, 2017 at 04:41:28PM +0100, Jakub Jelinek wrote: > > > On Thu, Jan 05, 2017 at 04:39:40PM +0100, Marek Polacek wrote: > > > > Coming back to this... > > > > > > > >

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On Mon, 9 Jan 2017, Marek Polacek wrote: > On Thu, Jan 05, 2017 at 04:41:28PM +0100, Jakub Jelinek wrote: > > On Thu, Jan 05, 2017 at 04:39:40PM +0100, Marek Polacek wrote: > > > Coming back to this... > > > > > > Right, after h0 == h1 is missing && operand_equal_p (thenb, elseb, 0) > > > > or

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On Thu, Jan 05, 2017 at 04:41:28PM +0100, Jakub Jelinek wrote: > On Thu, Jan 05, 2017 at 04:39:40PM +0100, Marek Polacek wrote: > > Coming back to this... > > > > Right, after h0 == h1 is missing && operand_equal_p (thenb, elseb, 0) > > > or so (the exact last operand needs to be figured out). >

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On January 5, 2017 4:41:28 PM GMT+01:00, Jakub Jelinek wrote: >On Thu, Jan 05, 2017 at 04:39:40PM +0100, Marek Polacek wrote: >> Coming back to this... > >> > Right, after h0 == h1 is missing && operand_equal_p (thenb, elseb, >0) >> > or so (the exact last operand needs to be

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On Thu, Jan 05, 2017 at 04:39:40PM +0100, Marek Polacek wrote: > Coming back to this... > > Right, after h0 == h1 is missing && operand_equal_p (thenb, elseb, 0) > > or so (the exact last operand needs to be figured out). > > OEP_ONLY_CONST is certainly wrong, we want the same VAR_DECLs to mean

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

Coming back to this... On Thu, Nov 03, 2016 at 02:38:50PM +0100, Jakub Jelinek wrote: > On Thu, Nov 03, 2016 at 09:27:55AM -0400, Jason Merrill wrote: > > On Thu, Nov 3, 2016 at 7:24 AM, Marek Polacek wrote: > > > On Tue, Nov 01, 2016 at 02:53:58PM +0100, Jakub Jelinek wrote:

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On 11/3/16, Jakub Jelinek wrote: > On Thu, Nov 03, 2016 at 09:27:55AM -0400, Jason Merrill wrote: >> On Thu, Nov 3, 2016 at 7:24 AM, Marek Polacek wrote: >> > On Tue, Nov 01, 2016 at 02:53:58PM +0100, Jakub Jelinek wrote: >> >> On Tue, Nov 01, 2016 at

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On Thu, Nov 03, 2016 at 09:27:55AM -0400, Jason Merrill wrote: > On Thu, Nov 3, 2016 at 7:24 AM, Marek Polacek wrote: > > On Tue, Nov 01, 2016 at 02:53:58PM +0100, Jakub Jelinek wrote: > >> On Tue, Nov 01, 2016 at 09:41:20AM -0400, Jason Merrill wrote: > >> > On Tue, Oct 25,

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On Thu, Nov 3, 2016 at 7:24 AM, Marek Polacek wrote: > On Tue, Nov 01, 2016 at 02:53:58PM +0100, Jakub Jelinek wrote: >> On Tue, Nov 01, 2016 at 09:41:20AM -0400, Jason Merrill wrote: >> > On Tue, Oct 25, 2016 at 9:59 AM, Marek Polacek wrote: >> > > On

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On Tue, Nov 01, 2016 at 02:53:58PM +0100, Jakub Jelinek wrote: > On Tue, Nov 01, 2016 at 09:41:20AM -0400, Jason Merrill wrote: > > On Tue, Oct 25, 2016 at 9:59 AM, Marek Polacek wrote: > > > On Mon, Oct 24, 2016 at 04:10:21PM +0200, Marek Polacek wrote: > > >> On Thu, Oct 20,

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On Tue, Nov 01, 2016 at 09:41:20AM -0400, Jason Merrill wrote: > On Tue, Oct 25, 2016 at 9:59 AM, Marek Polacek wrote: > > On Mon, Oct 24, 2016 at 04:10:21PM +0200, Marek Polacek wrote: > >> On Thu, Oct 20, 2016 at 12:28:36PM +0200, Marek Polacek wrote: > >> > I found a

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On Tue, Oct 25, 2016 at 9:59 AM, Marek Polacek wrote: > On Mon, Oct 24, 2016 at 04:10:21PM +0200, Marek Polacek wrote: >> On Thu, Oct 20, 2016 at 12:28:36PM +0200, Marek Polacek wrote: >> > I found a problem with this patch--we can't call >> > do_warn_duplicated_branches in

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

On Mon, Oct 24, 2016 at 04:10:21PM +0200, Marek Polacek wrote: > On Thu, Oct 20, 2016 at 12:28:36PM +0200, Marek Polacek wrote: > > I found a problem with this patch--we can't call > > do_warn_duplicated_branches in > > build_conditional_expr, because that way the C++-specific codes might leak >

Re: Implement -Wduplicated-branches (PR c/64279)

On 10/24/2016 08:44 AM, Marek Polacek wrote: On Mon, Oct 24, 2016 at 04:39:20PM +0200, Jakub Jelinek wrote: On Mon, Oct 24, 2016 at 04:34:40PM +0200, Marek Polacek wrote: On Mon, Oct 24, 2016 at 08:18:25AM -0600, Martin Sebor wrote: The case above is just a case where with -O GCC can figure

Re: Implement -Wduplicated-branches (PR c/64279)

On Mon, Oct 24, 2016 at 04:39:20PM +0200, Jakub Jelinek wrote: > On Mon, Oct 24, 2016 at 04:34:40PM +0200, Marek Polacek wrote: > > On Mon, Oct 24, 2016 at 08:18:25AM -0600, Martin Sebor wrote: > > > > The case above is just a case where with -O GCC can figure out what the > > > > value > > > >

Re: Implement -Wduplicated-branches (PR c/64279)

On Mon, Oct 24, 2016 at 04:34:40PM +0200, Marek Polacek wrote: > On Mon, Oct 24, 2016 at 08:18:25AM -0600, Martin Sebor wrote: > > > The case above is just a case where with -O GCC can figure out what the > > > value > > > of a const-qualified variable is, see > > >

Re: Implement -Wduplicated-branches (PR c/64279)

On Mon, Oct 24, 2016 at 08:18:25AM -0600, Martin Sebor wrote: > > The case above is just a case where with -O GCC can figure out what the > > value > > of a const-qualified variable is, see decl_constant_value_for_optimization. > > Since the warning is implemented even before gimplifying,

Re: Implement -Wduplicated-branches (PR c/64279)

On 10/24/2016 07:59 AM, Marek Polacek wrote: On Thu, Oct 20, 2016 at 02:21:42PM -0600, Martin Sebor wrote: --- gcc/testsuite/c-c++-common/Wduplicated-branches-1.c +++ gcc/testsuite/c-c++-common/Wduplicated-branches-1.c @@ -0,0 +1,187 @@ +/* PR c/64279 */ +/* { dg-do compile } */ +/* {

Re: Implement -Wduplicated-branches (PR c/64279) (v2)

On Thu, Oct 20, 2016 at 12:28:36PM +0200, Marek Polacek wrote: > I found a problem with this patch--we can't call do_warn_duplicated_branches > in > build_conditional_expr, because that way the C++-specific codes might leak > into > the hasher. Instead, I should use operand_equal_p, I think.

Re: Implement -Wduplicated-branches (PR c/64279)

On Thu, Oct 20, 2016 at 12:56:12PM -0600, Jeff Law wrote: > On 10/20/2016 08:37 AM, David Malcolm wrote: > > On Thu, 2016-10-20 at 16:24 +0200, Marek Polacek wrote: > > > On Thu, Oct 20, 2016 at 10:11:55AM -0400, Jason Merrill wrote: > > > > On Wed, Oct 19, 2016 at 7:07 AM, Marek Polacek

Re: Implement -Wduplicated-branches (PR c/64279)

On Thu, Oct 20, 2016 at 02:21:42PM -0600, Martin Sebor wrote: > > --- gcc/testsuite/c-c++-common/Wduplicated-branches-1.c > > +++ gcc/testsuite/c-c++-common/Wduplicated-branches-1.c > > @@ -0,0 +1,187 @@ > > +/* PR c/64279 */ > > +/* { dg-do compile } */ > > +/* { dg-options "-Wduplicated-branches

Re: Implement -Wduplicated-branches (PR c/64279)

--- gcc/testsuite/c-c++-common/Wduplicated-branches-1.c +++ gcc/testsuite/c-c++-common/Wduplicated-branches-1.c @@ -0,0 +1,187 @@ +/* PR c/64279 */ +/* { dg-do compile } */ +/* { dg-options "-Wduplicated-branches -O2" } */ + +extern void foo (int); +extern int g; +extern int a[10]; + +int +f (int

Re: Implement -Wduplicated-branches (PR c/64279)

On 10/20/2016 08:37 AM, David Malcolm wrote: On Thu, 2016-10-20 at 16:24 +0200, Marek Polacek wrote: On Thu, Oct 20, 2016 at 10:11:55AM -0400, Jason Merrill wrote: On Wed, Oct 19, 2016 at 7:07 AM, Marek Polacek wrote: But since integer csts and various decls don't carry

Re: Implement -Wduplicated-branches (PR c/64279)

On Thu, 2016-10-20 at 16:24 +0200, Marek Polacek wrote: > On Thu, Oct 20, 2016 at 10:11:55AM -0400, Jason Merrill wrote: > > On Wed, Oct 19, 2016 at 7:07 AM, Marek Polacek > > wrote: > > > But since integer csts and various decls > > > don't carry location info, this works

Re: Implement -Wduplicated-branches (PR c/64279)

On Thu, Oct 20, 2016 at 10:11:55AM -0400, Jason Merrill wrote: > On Wed, Oct 19, 2016 at 7:07 AM, Marek Polacek wrote: > > But since integer csts and various decls > > don't carry location info, this works only partially, so the warning > > warns even for e.g. > > > > if

Re: Implement -Wduplicated-branches (PR c/64279)

On Wed, Oct 19, 2016 at 7:07 AM, Marek Polacek wrote: > But since integer csts and various decls > don't carry location info, this works only partially, so the warning > warns even for e.g. > > if (TREE_STATIC (node) || DECL_EXTERNAL (node)) > max_align =

Re: Implement -Wduplicated-branches (PR c/64279)

I found a problem with this patch--we can't call do_warn_duplicated_branches in build_conditional_expr, because that way the C++-specific codes might leak into the hasher. Instead, I should use operand_equal_p, I think. Let me rework that part of the patch. Marek

Implement -Wduplicated-branches (PR c/64279)

This patch introduces a new warning, -Wduplicated-branches. Its purpose is to warn for code such as if (cond) return 0; else return 0; as well as e.g. r = i ? *p : *p; The approach I took was to use inchash::add_expr to hash the expressions and then compare the hashes. But of