From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11913 invoked by alias); 29 Aug 2014 20:27:50 -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 11897 invoked by uid 89); 29 Aug 2014 20:27:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: bastet.se.axis.com Received: from bastet.se.axis.com (HELO bastet.se.axis.com) (195.60.68.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 29 Aug 2014 20:27:47 +0000 Received: from localhost (localhost [127.0.0.1]) by bastet.se.axis.com (Postfix) with ESMTP id 51ABC18082; Fri, 29 Aug 2014 22:27:44 +0200 (CEST) Received: from bastet.se.axis.com ([IPv6:::ffff:127.0.0.1]) by localhost (bastet.se.axis.com [::ffff:127.0.0.1]) (amavisd-new, port 10024) with LMTP id cIOYyuU7Kip5; Fri, 29 Aug 2014 22:27:35 +0200 (CEST) Received: from boulder.se.axis.com (boulder.se.axis.com [10.0.2.104]) by bastet.se.axis.com (Postfix) with ESMTP id 752B018077; Fri, 29 Aug 2014 22:27:35 +0200 (CEST) Received: from boulder.se.axis.com (localhost [127.0.0.1]) by postfix.imss71 (Postfix) with ESMTP id 43F8EEC4; Fri, 29 Aug 2014 22:27:35 +0200 (CEST) Received: from seth.se.axis.com (seth.se.axis.com [10.0.2.172]) by boulder.se.axis.com (Postfix) with ESMTP id 383ACE5D; Fri, 29 Aug 2014 22:27:35 +0200 (CEST) Received: from ignucius.se.axis.com (ignucius.se.axis.com [10.88.21.50]) by seth.se.axis.com (Postfix) with ESMTP id 35C753E049; Fri, 29 Aug 2014 22:27:35 +0200 (CEST) Received: from ignucius.se.axis.com (localhost [127.0.0.1]) by ignucius.se.axis.com (8.12.8p1/8.12.8/Debian-2woody1) with ESMTP id s7TKRZQD011130; Fri, 29 Aug 2014 22:27:35 +0200 Received: (from hp@localhost) by ignucius.se.axis.com (8.12.8p1/8.12.8/Debian-2woody1) id s7TKRYu7011126; Fri, 29 Aug 2014 22:27:34 +0200 Date: Fri, 29 Aug 2014 20:27:00 -0000 Message-Id: <201408292027.s7TKRYu7011126@ignucius.se.axis.com> From: Hans-Peter Nilsson To: dmalcolm@redhat.com CC: gcc-patches@gcc.gnu.org In-reply-to: <1409335624.31600.32.camel@surprise> (message from David Malcolm on Fri, 29 Aug 2014 20:07:04 +0200) Subject: Re: [PATCH v2] Re: PR62304 (was Re: (Still) ICE for cris-elf at r214710) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=iso-8859-1 Content-Transfer-Encoding: 8BIT X-SW-Source: 2014-08/txt/msg02663.txt.bz2 > From: David Malcolm > Date: Fri, 29 Aug 2014 20:07:04 +0200 > BTW, in another email in the thread you said: > > > Thanks for the heads-up. BTW, the ChangeLog entries should say > > "what" not "why"; that goes into a comment in the source. > > OK. Where possible I've added comments in the new code, and chosen > variable names that express that we could be dealing with both insns and > RETURN nodes. Much of the patch is simply reverting changes made > earlier. Does it make sense to go in and add comments where I'm > reverting an change? Sure, if it makes a difference in understanding *compared to how things appear at first sight*. For example, assuming you *could* have made the comment "This should be of subtype X so it covers just case Y", then you'd change it to "While it seems this should be of subtype X to cover just case Y, it can't, because of Z, so we need to stick to the base-type W". Not sure if that makes sense here, of course; I didn't look closer. :) (Not to mention it's less obvious how to add comments to code that is just removed...) > I felt that adding stuff to a ChangeLog makes > sense from a "belt and braces" POV; if we have to review the change in 6 > months time, it's easier to have bonus "why", as it were. Yes, the question is just somewhere. The commit log works good enough that people will look there when they've found the "what" and want to know the "why" when both code and comments disappeared. > The attached patch fixes both reproducers you posted (tested with > cris-elf), and the case seen in PR62304 (tested with > sparc64-sun-solaris2.10); bootstrap on x86_64 in progress. > > It eliminates all uses of JUMP_LABEL_AS_INSN from reorg.c, and indeed > after that there are only 6 uses in the tree (including config subdirs). And build for cris-elf completes over into testing with this patch. Thanks! brgds, H-P