From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 67074 invoked by alias); 6 Oct 2015 22:43:51 -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 67064 invoked by uid 89); 6 Oct 2015 22:43:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham 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; Tue, 06 Oct 2015 22:43:49 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id B3877550CD for ; Tue, 6 Oct 2015 22:43:47 +0000 (UTC) Received: from [10.10.57.75] (vpn-57-75.rdu2.redhat.com [10.10.57.75]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t96Mhk9M023107; Tue, 6 Oct 2015 18:43:47 -0400 Subject: Re: [patch 4/3] Header file reduction - Tools for contrib To: Bernd Schmidt , gcc-patches@gcc.gnu.org References: <560DEA79.8050709@redhat.com> <56127AA4.9090707@redhat.com> <5612E939.9000701@redhat.com> <5613A1F9.3030407@codesourcery.com> <5613B846.2090107@redhat.com> <5613D4F0.7070204@redhat.com> <5613E13B.6010100@redhat.com> <56141EB0.7000001@redhat.com> <561430FB.1080301@redhat.com> From: Andrew MacLeod Message-ID: <56144EA2.1080109@redhat.com> Date: Tue, 06 Oct 2015 22:43:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <561430FB.1080301@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-10/txt/msg00647.txt.bz2 On 10/06/2015 04:37 PM, Bernd Schmidt wrote: > On 10/06/2015 09:19 PM, Andrew MacLeod wrote: >> I dont get your fear. I could have created that patch by hand, it would >> just take a long time, and would likely be less complete, but just as >> large. >> >> I'm not changing functionality. ALL the tool is doing is removing >> header files which aren't needed to compile. It goes to great pains to >> make sure it doesn't remove a silent dependency that conditional >> compilation might introduce. Other than that, the sanity check is that >> everything compiles on every target and regression tests show nothing. >> Since we're doing this with just include files, and not changing >> functionality, Im not sure what your primary concern is? > > My concern is that I've seen occasions in the past where "harmless > cleanups" that were not intended to alter functionality introduced > severe and subtle bugs that went unnoticed for a significant amount of > time. If a change does not alter functionality, then there is a valid > question of "why apply it then?", and the question of correctness > becomes very important (to me anyway). The patch was produced by a > fairly complex process, and I'd want to at least be able to convince > myself that the process is correct. > > Anyhow, I'll step back from this, you're probably better served by > someone else reviewing the patch. > > > Bernd I do get it. And I have spent a lot of time trying to make sure none of those sort of bugs come in, and ultimately have tried to be conservative.. after all, its better to have the tool leave an include than remove one that may be required. Ultimately, these changes are unlikely to introduce an issue, but there is a very slight possibility. Any issues that do surface should be of the "not using a pattern" kind because a conditional compilation code case was somehow missed. I'm hoping for none of those obviously. Anyway, the tool does seem to work on all the tests I have looked at. If any bugs are uncovered by this, then they are also latent issues we didn't know about that should be exposed and fixed anyway. I am fine if we'd like to separate the patches into the reordering, and the deleting. Its not a lot of effort on my part, just a lot of time compiling for the reducer in the background.. and we can do them as 2 commits if that is helpful. What I don't want to do is spend a lot more time massaging the tools for contrib because I am sick of looking at them right now, and no one is in a hurry to use them anyway... if anyone ever does.:-) The documentation grammer should certainly be fixed up and I will add some comments around the questions you had. we could also do a small scale submission on half a dozen files, provide the reorder patch, and then the reduction patch with the logs if that helps whoever is reviewing get comfortable with what the tool is doing, then its easier to simply acknowledge the mechanical nature of the large commit. Perhaps it would be educational anyway. I'll do it however you guys want... i just want to get it done :-) Andrew