This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [patch 4/3] Header file reduction - Tools for contrib
- From: Bernd Schmidt <bschmidt at redhat dot com>
- To: Andrew MacLeod <amacleod at redhat dot com>, gcc-patches at gcc dot gnu dot org
- Date: Tue, 6 Oct 2015 16:56:59 +0200
- Subject: Re: [patch 4/3] Header file reduction - Tools for contrib
- Authentication-results: sourceware.org; auth=none
- References: <560DEA79 dot 8050709 at redhat dot com> <56127AA4 dot 9090707 at redhat dot com> <5612E939 dot 9000701 at redhat dot com> <5613A1F9 dot 3030407 at codesourcery dot com> <5613B846 dot 2090107 at redhat dot com> <5613D4F0 dot 7070204 at redhat dot com>
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 suspect you'll have to regenerate the includes patch anyway, because
of the missing #undef tracking I mentioned.
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'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).
Bernd