Re: patch rereview requested for PRs 6860, 10467 and 11741

[Roger Sayle <roger at eyesopen dot com>]

On 21 Sep 2003, Gabriel Dos Reis wrote:
> | All a reviewer has to do to screw up the entire process, is to hit
> | reply without fully reading the posting and write "I don't think
> | it works".
>
> I do not believe all reviewers, especially RTH, have a secret plan to
> screw up works.

Let me make it very explicitly clear that this isn't a personal attack
against RTH.  In the circa 285 patches, I've submitted to GCC, I'd have
to say that less than 25 have ever been reviewed by anyone other than
RTH.  The fact that, by the same calculation, he single handedly reviews
over 90% of the patches to the middle-end and RTL optimizers makes it
unfortunate his name becomes associated with the three or four patches
of mine that I feel have been poorly reviewed.  Indeed, this rates RTH
at over 98% customer satisfaction.

Don't make the mistake that me questioning the most controversial 2%
reflects, anything other than awe at RTH's abilities.  Its very much
an honor and priviledge to be able to collaborate with him on GCC.
I hate to type it out loud, but he's easily the best software engineer
I've ever worked with.


It is interesting to note however that of all my patches hung up in
review, every  one has been commented on by either Jeff Law, Richard
Kenner or Geoff Keating (not to name any names:>).  Perhaps the
contributions of other reviewers adversely affects RTH's judgement :>


> Patches must be assumed innocent until proven guilty!
>

I still stand by these words.  Once a patch has satisfied all the
prerequisites specified in contributing.html, including regression
tests and full explanation of why it works, that should be sufficient.
Neither submitter nor reviewer should be considered at fault, if "best
practice" has been observed, even if the reviewer doesn't understand
the changes but agrees they are compatible with the goals of the GCC
project.

Remember that if a regression is subsequently found, including latent bugs
in completely unrelated parts of the compiler or only triggered on obscure
platforms, it is the patch submitter and not the patch reviewer who is
responsible for fixing the new failure, or reverting the original patch.
There is no burden placed upon the reviewer (though it would be nice if
he/she could approve the fix in a timely manner, i.e. within the 48 hour
rule).


> Patches must be assumed innocent until proven guilty!
>

Again it is unreasonable to hold up a patch that in itself is completely
correct, under the fear that there is or maybe a bug elsewhere in the
compiler.  Bugzilla is full of bugs known to be elsewhere in the compiler.
If we can't clean up the code one bit at a time, there would be complete
impasse.  Once a patch has been shown not to break anything by the best
of our knowledge, it should be assumed innocent until proven guilty.

If removing an ICE causes broken code to be generated silently elsewhere
in th compiler, clearly the "elsewhere in the compiler" is broken and
needs to be fixed.  Covering up or hiding these bugs by holding perfectly
valid patches means they'll never be found.


> No, consider what your line of rhetoric may lead to:
>
>    If someone with RTH's history of regression fixing patches, GCC design
>    and code improvements can't reject a patch his knowledge of the
>    compiler, what's the world coming to?
>
> Sure, it sounds nasty.  Doesn't it?

As I said above, if I agree without comment to 98% of RTH's reviews,
does that mean I'm unworthy to question his judgement every once in
a while.  Surely, no-one is beyond peer-review, even global write
priviledge maintainers?

Or put another way, If someone with RTH's history of regression fixing
patches, GCC design and code improvements can't come up with a failing
test case, isn't that an incredibly strong argument that the patch is
sound?

All I asked is that if RTH believes he can come up with an example that
would demonstrate the failings of a patch, that it would be great if he
could do so, so we can make some form of progress on these high priority
PRs.


If GCC maintainers really were omnipotent, there would be no bugs for
me to fix...

Roger
--