Withdrawn patches

[Roger Sayle]

On 4 Dec 2003, Richard Sandiford wrote:
> > Well that's unfortunate!  I'd read through both of these patches but
> > it wasn't clear if they were only addressing hypothetical latent bugs.
> > Neither included a testcase for GCC's regression suite nor mentioned a
> > bugzilla PR.
>
> Err... the first one did.  Search for gcc.c-torture/execute/20030916-1.c
> in the message linked above.  And the problem that the testcase exposes
> is described in the covering message.

My apologies.  I completely missed that in the middle of the diff.  It
wasn't mentioned in either ChangeLog and didn't appear at the end of the
posting.  I don't normally go through every line of a contribution until
after I've checked whether its relevant for stage3, and in this case
I screwed up.


> The second one didn't because I didn't know about the bug at the
> time either. ;)  It was originally submitted an optimisation,
> not a bug fix.  But I think the message explained what the patch
> was trying to achieve.

I certainly appreciate how difficult it is to write "portable" test
cases for GCC's middle-end and RTL optimizers.  However, I'm just
a bit cautious during stage3, and if it isn't possible to check that
a patch is doing the correct thing, and that there isn't a bad latent
interaction with some other pass or on another platform, then my
personal opinion is that it can wait until 3.5 rather than risk
destablizing the tree.

For example, only this morning you decided for similar reasons to
revert your direct_pool_load_p patch.
http://gcc.gnu.org/ml/gcc-patches/2003-12/msg00446.html
Again neither this patch, nor its reversion included testcases,
leaving mainline in the state that it won't obviously be clear
that this problem was ever resolved and whether it still exists.


> > They'd have been approved almost immediately if you'd included this
> > information in your original postings.
>
> Not sure I understand what you mean here.  It sounds like, if I'd
> filed a bugzilla report saying "loop optimisers don't recognise
> promoted bivs", and cut&paste the covering message for my second
> patch into it, the patch would somehow have been easier to review
> than it was by including the description with the patch itself...


I apologise for being unclear, so I thought I'd explain try and explain
how I personally prioritize and review patches.

Firstly, let me stress that these are just my personal opinions.  By
my reckoning there are thirteen maintainers/reviewers how can approve
patches to GCC's RTL optimizers, and I'm just the most junior.

Its clear that during different stages of GCC's development cycle, and
even early in a stage vs. late in a stage, affects the conditions under
which patch is approved.  During stage1 or stage2, I'd have no hisitation
at all in approving the patches above, but as we near a release, the
benefit vs. risk needs to be more carefully assessed.  If I'm unsure,
the "safest" course is to leave a patch to a more experienced reviewer.

For me personally, a testcase that comes with a patch counts for a huge
amount.  A new deja-gnu test, even if only on a single platform, clearly
demonstrates that a bug has been fixed and that the patch is "obviously"
suitable even during stage3 or on a release branch.  Admittedly, its
possible to fix bugs without a testcase, but lack of a testcase makes
for a less clear-cut decision.  If in doubt, leave until a thaw or for
a reviewer more confident in that part of the code.


I know Mark and some other reviewers require that a PR be filed before
reviewing a patch as we approach a release.  To me this isn't as
important as clearly describing the problem and solution when posting
a patch, and providing a self-contained test.  The advantage of the
PR route, is that it forces people to clearly describe the problem and
provide a reproducable test case, listing the platforms and gcc version
numbers it affects.  GCC's "bugzilla" maintainers do an impressive job
of ensuring that all the necessary information is provided, testing
platforms, GCC versions, reducing testcases, assigning target milestones,
distinguishing enhancement requests, from ICE on illegal, from serious
issues.

So when I say that I prioritize reviews of patches containing tests
or PR numbers, its not the actual filing in bugzilla that's relevant,
rather than that it guarantees a test case :>  I often bootstrap and
regression test proposed patches as part of the review process, but
without even a piece of code that triggers the code modified in the
patch, I can't even step through it with a debugger.


Again my apologies for not responding to your patches or David's ping.
I was waiting to see if anyone else would approve them before the thaw,
at which point I'd OK them myself.  I'd missed the bug-fixing nature
of these changes.


Much of the above is based on my paranoia and confidence in my role
as maintainer, so I thought I'd defend my current protocols.  Perhaps
in a few years, if I can approach the skills of RTH, I'll be able to
look at a patch, see the streams of green characters rolling down a
black screen, and approve complete rewrites of reload.c, without even
a single hesitation.


Sorry,

Roger
--