From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 82792 invoked by alias); 18 Sep 2015 10:06:13 -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 82775 invoked by uid 89); 18 Sep 2015 10:06:13 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 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; Fri, 18 Sep 2015 10:06:12 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 60EEE8B136; Fri, 18 Sep 2015 10:06:11 +0000 (UTC) Received: from redhat.com (ovpn-204-20.brq.redhat.com [10.40.204.20]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t8IA66ZU022524 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NO); Fri, 18 Sep 2015 06:06:09 -0400 Date: Fri, 18 Sep 2015 10:24:00 -0000 From: Marek Polacek To: Martin Sebor Cc: GCC Patches , Jason Merrill , Joseph Myers Subject: Re: [C/C++ PATCH] RFC: Implement -Wduplicated-cond (PR c/64249) Message-ID: <20150918100606.GF27588@redhat.com> References: <20150916155915.GA27588@redhat.com> <55F9D820.1050902@gmail.com> <20150917160538.GD27588@redhat.com> <55FAEC54.1070508@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55FAEC54.1070508@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-SW-Source: 2015-09/txt/msg01386.txt.bz2 On Thu, Sep 17, 2015 at 10:37:40AM -0600, Martin Sebor wrote: > >>The patch currently issues a false positive for the test case > >>below. I suspect the chain might need to be cleared after each > >>condition that involves a side-effect. > >> > >> int foo (int a) > >> { > >> if (a) return 1; else if (++a) return 2; else if (a) return 3; > >> return 0; > >> } > > > >But the last branch here can never be reached, right? If a == 0, foo > >returns 2, otherwise it just returns 1. So I think we should diagnose > >this. > > It probably wasn't the best example. The general issue here is > that the second condition has a side-effect that can change (in > this case clearly does) the value of the expression. > > Here's a better example: > > int a; > > int bar (void) { a = 1; return 0; } > > int foo (void) { > if (a) return 1; > else if (foo ()) return 2; > else if (a) return 3; > return 0; > } > > Since we don't know bar's side-effects we must assume they change > the value of a and so we must avoid diagnosing the third if. Ok, I'm convinced now. We have something similar in the codebase: libsupc++/eh_catch.cc has int count = header->handlerCount; if (count < 0) { // This exception was rethrown. Decrement the (inverted) catch // count and remove it from the chain when it reaches zero. if (++count == 0) globals->caughtExceptions = header->nextException; } else if (--count == 0) { // Handling for this exception is complete. Destroy the object. globals->caughtExceptions = header->nextException; _Unwind_DeleteException (&header->unwindHeader); return; } else if (count < 0) // A bug in the exception handling library or compiler. std::terminate (); Here all arms are reachable. I guess I need to kill the chain of conditions when we find something with side-effects, exactly as you suggested. Again, thanks for your comments. Marek