From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 58703 invoked by alias); 6 Oct 2015 19:19:17 -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 58694 invoked by uid 89); 6 Oct 2015 19:19:17 -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 19:19:15 +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 371F2AE5C4 for ; Tue, 6 Oct 2015 19:19:14 +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 t96JJDIx016445; Tue, 6 Oct 2015 15:19:13 -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> From: Andrew MacLeod Message-ID: <56141EB0.7000001@redhat.com> Date: Tue, 06 Oct 2015 19:19: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: <5613E13B.6010100@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-10/txt/msg00630.txt.bz2 On 10/06/2015 10:56 AM, Bernd Schmidt wrote: > On 10/06/2015 04:04 PM, Andrew MacLeod wrote: > >> I primarily submitted it early because you wanted to look at the tools >> before the code patch, which is the one I care about since the longer it >> goes, the more effort it is to update the patch to mainline. > > The problem is that the generated patch is impossible to review on its > own. It's just a half a megabyte dump of changes that can't > realistically be verified for correctness. Reading it can throw up > some interesting questions which can then (hopefully) be answered by > reference to the tools, such as "why does timevar.h move?" For that to > work, the tools need at least to have a minimum level of readability. > They are the important part here, not the generated patch. (Unless you > find a reviewer who's less risk-averse than me and is willing to > approve the whole set and hope for the best.) 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? You are unlikely to ever be able to read the patch and decide for yourself whether removing expr.h from the header list is correct or not. Much like if I proposed the same thing by hand. Yes, I added the other tool in which reorders the headers and removes duplicates, and perhaps that is what is causing you the angst. The canonical ordering was developed by taking current practice and adding in other core files which had ordering issues that showed up during the reduction process. Reorderiing all files to this order should actually resolve more issues than it causes. I can generate and provide that as a patch if you want to look at it separately... I dont know what that buys you. you could match the includes to the master list to make sure the tool did its job by itself I guess. The tools are unlikely to ever be used again... Jeff suggested I provide them to contrib just in case someone decided to do something with them someday, they wouldn't be lost,or at least they wouldn't have to track me down to get them. IF we discover that one or more of the tools does continue to have some life, well then maybe at that point its worth putting some time into refining it a bit better. > I suspect you'll have to regenerate the includes patch anyway, because > of the missing #undef tracking I mentioned. I dont see that #undef is relevant at all. All the conditional dependencies care about is "MAY DEFINE" Its conservative in that if something could be defined, we'll assume it is and not remove any file which may depend on it. to undefine something in a MAY DEFINE world doesnt mean anything. > > Let's consider the timevar.h example a bit more. Does the include have > to move? I don't see anything in that file that looks like a > dependency, and include files that need it are already including it. > Is the fact that df.h includes it in any way material for generating > an order of headers? IMO, no, it's an unnecessary change indicating a > bug in the script, and any kind of unnecessary change in a patch like > this makes it so much harder to verify. I think the canonical order > that's produced should probably ignore files included from other > headers so that these are left alone in their original order. > I covered this in the last note. Pretty much every file is going to have a "core" of up to 95 files reordered into the canonical form, which taken as a snapshot of any given file, may look arbitrary but is in fact a specific subset of the canonical ordering. You cant only order some parts of it because there are subtle dependencies between the files which force you to look at them all. Trust me, I didnt start by reordering all of them this way... it developed over time. > I'd still like more explanations of special cases in the tools like > the diagnostic.h area as well as > # seed tm.h with options.h since its a build file and won't be seen. > and I think we need to understand what makes them special in a way > that makes the rest of the algorithm not handle them correctly (so > that we don't overlook any other such cases). > See the other note, its because of the front end files/diagnostic dependencies or irreconcilable cycles because of what a header includes. Any other case would have shown up the way those did during development. Andrew