On 10/06/2015 04:37 PM, Bernd Schmidt wrote:
On 10/06/2015 09:19 PM, Andrew MacLeod wrote:
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?
My concern is that I've seen occasions in the past where "harmless
cleanups" that were not intended to alter functionality introduced
severe and subtle bugs that went unnoticed for a significant amount of
time. If a change does not alter functionality, then there is a valid
question of "why apply it then?", and the question of correctness
becomes very important (to me anyway). The patch was produced by a
fairly complex process, and I'd want to at least be able to convince
myself that the process is correct.
Anyhow, I'll step back from this, you're probably better served by
someone else reviewing the patch.
Bernd
I do get it. And I have spent a lot of time trying to make sure none of
those sort of bugs come in, and ultimately have tried to be
conservative.. after all, its better to have the tool leave an include
than remove one that may be required.
Ultimately, these changes are unlikely to introduce an issue, but there
is a very slight possibility. Any issues that do surface should be of
the "not using a pattern" kind because a conditional compilation code
case was somehow missed. I'm hoping for none of those obviously.
Anyway, the tool does seem to work on all the tests I have looked at.
If any bugs are uncovered by this, then they are also latent issues we
didn't know about that should be exposed and fixed anyway.
I am fine if we'd like to separate the patches into the reordering, and
the deleting. Its not a lot of effort on my part, just a lot of time
compiling for the reducer in the background.. and we can do them as 2
commits if that is helpful.
What I don't want to do is spend a lot more time massaging the tools for
contrib because I am sick of looking at them right now, and no one is in
a hurry to use them anyway... if anyone ever does.:-) The documentation
grammer should certainly be fixed up and I will add some comments around
the questions you had.
we could also do a small scale submission on half a dozen files, provide
the reorder patch, and then the reduction patch with the logs if that
helps whoever is reviewing get comfortable with what the tool is doing,
then its easier to simply acknowledge the mechanical nature of the large
commit.
Perhaps it would be educational anyway.
I'll do it however you guys want... i just want to get it done :-)
Andrew
