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.
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.
* 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.
Let's take this example:
Index: attribs.c
===================================================================
*** attribs.c (revision 228331)
--- attribs.c (working copy)
*************** along with GCC; see the file COPYING3.
*** 20,36 ****
#include "config.h"
#include "system.h"
#include "coretypes.h"
! #include "tm.h"
#include "tree.h"
- #include "alias.h"
#include "stringpool.h"
#include "attribs.h"
#include "stor-layout.h"
- #include "flags.h"
- #include "diagnostic-core.h"
- #include "tm_p.h"
- #include "cpplib.h"
- #include "target.h"
#include "langhooks.h"
#include "plugin.h"
--- 20,31 ----
#include "config.h"
#include "system.h"
#include "coretypes.h"
! #include "target.h"
#include "tree.h"
#include "stringpool.h"
+ #include "diagnostic-core.h"
#include "attribs.h"
#include "stor-layout.h"
#include "langhooks.h"
#include "plugin.h"
You could be misled into thinking that diagnostic-core.h and target.h
are removed, and the algorithm confuses the issue by showing that lines
"changed" rather than getting removed or added. With a unified diff, the
size is cut in half which makes things more readable to begin with:
Index: attribs.c
===================================================================
--- attribs.c (revision 228331)
+++ attribs.c (working copy)
@@ -20,17 +20,12 @@
#include "config.h"
#include "system.h"
#include "coretypes.h"
-#include "tm.h"
+#include "target.h"
#include "tree.h"
-#include "alias.h"
#include "stringpool.h"
+#include "diagnostic-core.h"
#include "attribs.h"
#include "stor-layout.h"
-#include "flags.h"
-#include "diagnostic-core.h"
-#include "tm_p.h"
-#include "cpplib.h"
-#include "target.h"
#include "langhooks.h"
#include "plugin.h"
and with things closer together it's easier to follow what's actually
going on. This is a smaller example, many instances in your patch are
actually about a page long and you have to scroll back and forth to work
things out, getting confused because everything in the 410k of text
looks the same.
This was actually one of the reasons I proposed splitting the patch into
reordering and removal phases, it would alleviate the diff -c disadvantages.
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.).
Bernd
