From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 92033 invoked by alias); 17 Sep 2015 16:36:35 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 92018 invoked by uid 89); 17 Sep 2015 16:36:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 17 Sep 2015 16:36:34 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 48D9B2F02DC; Thu, 17 Sep 2015 16:36:33 +0000 (UTC) Received: from localhost.localdomain (ovpn-113-28.phx2.redhat.com [10.3.113.28]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t8HGaWj0030324; Thu, 17 Sep 2015 12:36:32 -0400 Subject: Re: [C/C++ PATCH] RFC: Implement -Wduplicated-cond (PR c/64249) To: Marek Polacek , Martin Sebor References: <20150916155915.GA27588@redhat.com> <55F9D820.1050902@gmail.com> <20150917160538.GD27588@redhat.com> Cc: GCC Patches , Jason Merrill , Joseph Myers From: Jeff Law Message-ID: <55FAEC10.3070100@redhat.com> Date: Thu, 17 Sep 2015 16:37:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <20150917160538.GD27588@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-09/txt/msg01308.txt.bz2 On 09/17/2015 10:05 AM, Marek Polacek wrote: > >> The patch doesn't diagnose some more involved cases like the one >> below: >> >> if (i < 0) return 1; else if (!(i > 0 || i == 0)) return 2; >> >> even though it does diagnose some other such cases, including: >> >> if (i < 0) return 1; else if (!(i >= 0)) return 2; >> >> and even: >> >> int foo (int a, int b, int c) { >> if (a + c == b) return 1; else if (a + c - b == 0) return 2; >> return 0; >> } >> >> I'm guessing this is due to operand_equal_p returning true for >> some pairs of equivalent expressions and false for others. Would >> making this work be feasible? > > You're right that this is limited by what operand_equal_p considers equal and > what not. I'm not sure if there is something more convoluted else I could use > in the FE, but I think not. It certainly doesn't look terrible important to > me. And you'll run into a point of diminishing returns quickly. There's many ways to write this stuff in ways that are equivalent, but a pain to uncover. I think relying on operand_equal_p is probably sufficient at this stage. > >> You probably didn't intend to diagnose the final else clause in >> the following case but I wonder if it should be (as an extension >> of the original idea): >> >> if (i) return 1; else if (!i) return 2; else return 0; > > Diagnosing this wasn't my intention. I'm afraid it would be kinda hard > to do that. This is the "unreachable code" problem. We used to have a warning for that, but in practice it was so unstable it was removed. > >> (In fact, I wonder if it might be worth diagnosing even the >> 'if (!i)' part since the condition is the inverse of the one >> in the if and thus redundant). > > This, on the other hand, seems doable provided we have some predicate that > would tell us whether an expression is a logical inverse of another expression. > E.g. "a > 3" and "a <= 3". Something akin to pred_equal_p -- invert one expr > and check whether they're equal. I think you just want to select a canonical form. So you canonicalize, then compare. jeff