This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Sun, 21 Sep 2003, Richard Kenner wrote:
>     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.
>
> I strongly disagree.  Those are *minimum requirements*, nothing more.
> There can be numerous reasons why patches that meet these requirements
> are not acceptable.

Agreed.  As you point out later in your posting "it must also improve
something".  A reviewer may ask for the patch to be tested on particular
targets, such as a cc0 machine or check there isn't a code size increase
on s390, etc...  If a reviewer feels that a patch needs to meet more than
the *minimum requirements*, he/she should explain precisely what those
additional requirements are.  A good reviewer should be able to describe
the requirements necessary to allay his/hear fears, or cautiously accept
that there is an inherent risk in any patch.


>     even if the reviewer doesn't understand the changes but agrees
>     they are compatible with the goals of the GCC project.
>
> A major goal of the GCC project is to create a piece of software that
> can be cooperatively maintained.  I'm not talking about your patch
> specifically (or any other ones), but if the reviewer can't understand
> the changes, it's unlikely that people reading the code will be able to
> understand it either and understanding the code is a critical part of
> the success of the GCC project.

Understanding in this context is more than realizing what the code does.
I'm sure you and RTH understand every line of my proposed patch.  The
more general "understanding" is the potential interactions with other
parts of the compiler.  It is because the GCC project is cooperatively
maintained, in addition to being so large, that no single person can
completely understand the myriad interactions that go on inside it.
There are even parts of the compiler, some parts of reload, g77's parser
or some of the unmaintained backends, where few if any of GCC's current
maintainers understand even how the current code is supposed to work.

It should also be appreciated that many of the write-after-approval
maintainers, front-end maintainers and back-end maintainers may be far
more familiar with the pieces of code they are actively contributing
too, than the global write maintainer that needs to approve their changes.
In these circumstances too, a maintainer may not completely "understand"
the implications of a change, but must use some level of trust in the
contributor's abilities.


> No.  If I have a gut feeling that something is wrong and I ask a submitter
> to show me why I'm wrong, it's unreasonable to expect that I be able to
> come up with a test case.

Might I recommend Zantac and cutting down on spicy foods?  One would hope
that when you ask a submitter "why you're wrong", you provide more to go
on than just a "feeling in your gut".

A test case is just one of the ways in which a reviewer may demonstrate
or explain potential failings in a patch. Back to my first paragraph
above, there are plenty of additional  requirements that a reviewer can
ask of a submitter. But just asking "Can you guarantee that some
unpredictable iteraction caused by your change will not eventually lead
to a problem in the compiler?" is in my opinion a "poor" review.  Indeed,
this is a question both reviewer/submitter will always struggle with but
ultimately can never be adequately addressed a priori.

Not that these "poor reviews" don't occur, just they are less helpful than
the constructive kind.  They're also less advancing than "Lets commit it
to the development tree in stage1, let everyone pound on it and if there
are no problems after a few weeks, it can stay."  Even a patch approval
doesn't guarantee you more than 48 hours in the tree.


Anyway I've said my peace.  If people disagree that it would be helpful
if reviewers explain clearly why they disapprove of patches, then I have
nothing more to add to the argument that I haven't already posted.  I'll
just agree to disagree.

Roger
--


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]