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
