[patch 4/3] Header file reduction - Tools for contrib

[Andrew MacLeod]

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