On 10/05/2015 04:37 PM, Bernd Schmidt wrote:
On 10/05/2015 10:10 PM, Andrew MacLeod wrote:
Its just an example of the sort of redundant includes the tool removes.
And your assertion turns out to be incorrect... bitmap.h is barely used
outside the backend, thus it is included in the backend.h aggregator
(This is the only header now which includes bitmap.h... Most of this
many-month effort was to untangle all those indirect includes.)
I said a few headers include obstack.h, not bitmap.h, and that's true
in my (maybe a week old) checkout. My suggestion was to move the
include of the former to (as Richi corrected) coretypes.h.
Ah, sorry the parsing was non-deterministic and I parsed it the other
way. My comments refer to the "true" dependencies in the code after
all the un-needed headers have been trimmed out... i've little doubt
mainline shows obstack.h and bitmap.h being included everywhere.
In any case, a direct include of obstack.h in coretypes.h was considered
earlier in the aggregation process and it didn't show up as something
that would be a win. It is included a couple of common places that we
have no control over.. in particular libcpp/include/symtab.h includes
obstack.h and is included by tree-core.h. A very significant number of
files bring that in. If we included obstack.h in coretypes.h then those
files would be including it again for a second time for no particularly
good reason. So I made the judgement call to not put it in coretypes.h.
And it's one example, but it does point out a problem with this sort
of automated approach: realistically no one is going to check the
whole patch, and it may contain changes that could be done better.
The point being that the aggregation *wasn't* automated... and has
nothing to do with this patch set. I analyzed and preformed all that
sort of thing earlier. Sure judgment calls were made, but it wasn't
automated in the slightest. There are certainly further aggregation
improvements that could be made... and either I or someone else could do
more down the road., The heavy lifting has all been done now.
So the *only* thing that is automated is removing include files which
are not needed so that we can get an idea of what the true dependencies
in the source base are.
* diff -c is somewhat unusual and I find diff -u much more readable.
unsual? I've been using -cp for the past 2 decades and no one has ever
mentioned it before... poking around the wiki I see it mentions you
can use either -up or -cp.
I guess I could repackage things using -up... I don't even know where
my script is to change it :-). is -u what everyone uses now? no one
has mentioned it before that I am aware of.
I'm pretty much used to seeing diff -u, whenever I get a -c diff
things become harder to work out, because the region in the diff
you're looking at never tells you the full story. In this case in
particular, the existence of both reordering and removing changes
makes it very hard to mentally keep track of what's going on.
I can switch to -u.. I've just never seen the request before.
I can regenerate the patches with -u if you want.
the reduction on all those files will take the better part of a week.
That's a little concerning due to the possibility of intervening
commits. I'd like to make one requirement for checkin, that you take
the revision at which you're committing and then run the script again,
verifying that the process produces the same changes as the patch you
committed. (Or do things in smaller chunks.).
Well, sure there are intervening commits.. the only ones that actually
matter are the ones which fail to compile because someone made a code
change which now requires a header that wasn't needed before. which is
really a state we're looking for I think. I fix those up before
committing. Its *possible* a conditional compilation issue could creep
in, but highly unlikely.
I can rerun everything on that revision from just before I committed and
see if everything is the same. It'll take a week to find out :-) but
that seems like a reasonable sanity check.
Andrew
