mirror of
https://github.com/Mercury-Language/mercury.git
synced 2026-04-21 20:33:55 +00:00
ea57900b24
Estimated hours taken: 0.3
Update the review procedures to reflect what we are doing now, which
seems to be working well.
compiler/notes/reviews.html:
Add different subjects ("for review" "diff" and "trivial diff"
for different classes of reviewing).
Allow second round diffs to be committed before second review,
if there were few problems at the first review.
287 lines
8.9 KiB
HTML
287 lines
8.9 KiB
HTML
|
|
<html>
|
|
<head>
|
|
<title>
|
|
Reviews
|
|
</title>
|
|
</head>
|
|
|
|
<body
|
|
bgcolor="#ffffff"
|
|
text="#000000"
|
|
>
|
|
|
|
|
|
<hr>
|
|
<!----------------------->
|
|
|
|
<h1> Reviews </h1> <p>
|
|
|
|
This file outlines the policy on reviews for the Mercury system.
|
|
|
|
<hr>
|
|
<!----------------------->
|
|
|
|
<h2> Reviewable material </h2>
|
|
|
|
<p>
|
|
|
|
All changes to the Mercury repository, including the compiler,
|
|
documentation, www pages, library predicates, runtime system, and tools
|
|
need to be reviewed.
|
|
|
|
<p>
|
|
|
|
<h2> Review process </h2>
|
|
|
|
<ol>
|
|
<li> Make sure you are working with an up-to-date copy of the
|
|
module you are using.
|
|
<li> If change is a code change, test change. See "Testing" section
|
|
of coding standards. Testing may take time - don't forget
|
|
that steps 3, 4 and 5 can be done in parallel.
|
|
<li> Create diff - use `cvs diff -u'. New files should be
|
|
appended verbatim to the end of the diff, with descriptions
|
|
indicating the name of the file.
|
|
<li> Write log message for this change - use template (see below).
|
|
<li> Review diff and log message yourself. (see below)
|
|
<li> Send to mercury-developers@cs.mu.oz.au, with the subject
|
|
"for review: <short description of change>".
|
|
Nominate a reviewer at top of diff (see below).
|
|
(If this change has been reviewed once before, it might
|
|
fall into the "commit before review" category -- see the
|
|
section on exceptions).
|
|
<li> Wait for review (see below).
|
|
<li> Fix any changes suggested.
|
|
<li> Repeat above steps until approval.
|
|
<li> Commit change (see below).
|
|
</ol>
|
|
|
|
|
|
<h2> Log Messages </h2>
|
|
|
|
Use the template that cvs provides.
|
|
|
|
<pre>
|
|
Estimated hours taken: _____
|
|
|
|
<overview or general description of changes>
|
|
|
|
<directory>/<file>:
|
|
<detailed description of changes>
|
|
</pre>
|
|
|
|
In estimated hours, include all your time to fix this problem -
|
|
including debugging time.
|
|
|
|
<p>
|
|
|
|
The description should state why the changes were made, not just what
|
|
the changes were. All file modifications related to the same change
|
|
should be committed together, and use the same log message, even over
|
|
multiple directories. The reason for this is that the log messages can
|
|
be viewed on a file-by-file basis, and it is useful to know that a small
|
|
change of a file in a subdirectory is related to a larger change in
|
|
other subdirectories.
|
|
|
|
<p>
|
|
|
|
For very small changes, the <overview or general description> can be
|
|
omitted, but the <detailed description> should stay.
|
|
|
|
<p>
|
|
|
|
If adding a new feature, this is a good place to describe the feature,
|
|
how it works, how to turn it on and off, and any present limitations of
|
|
the feature (note that all this should also be documented within the
|
|
change, as well). If fixing a bug, describe both the bug and the fix.
|
|
|
|
<p>
|
|
|
|
<h2> Self-Review </h2>
|
|
|
|
<p>
|
|
|
|
You should also review your own code first, and fix any obvious
|
|
mistakes. Where possible add documentation - if there was something you
|
|
had to understand when making the change, document it - it makes it
|
|
easier to review the change if it is documented, as well as generally
|
|
improving the state of documentation of the compiler.
|
|
|
|
<p>
|
|
|
|
<h2> Review </h2>
|
|
|
|
<p>
|
|
|
|
We're now posting all diffs to mercury-developers@cs.mu.oz.au.
|
|
|
|
<p>
|
|
|
|
The reasons for posting to mercury-developers are:
|
|
|
|
<ul>
|
|
<li> To increase everyone's awareness of what changes are taking
|
|
place.
|
|
<li> Give everyone interested a chance to review your code, not
|
|
just the reviewer. Remember, your changes may impact upon
|
|
the uncommitted work of others, so they may want to give
|
|
input.
|
|
<li> Allow other people to read the reviewer's comments - so the same
|
|
problems don't have to be explained again and again.
|
|
<li> People can try to see how your changes worked without having
|
|
to figure out how to get cvs to generate the right set of
|
|
diffs.
|
|
<li> Important decisions are often made or justified in reviews, so
|
|
these should be recorded.
|
|
</ul>
|
|
|
|
You should try to match the reviewer to the code - someone familiar with
|
|
a section of code can review faster and is more likely to catch errors.
|
|
Put a preamble at the start of your diff to nominate who you would like
|
|
to review the diff.
|
|
|
|
<p>
|
|
|
|
<h2> Waiting and approval </h2>
|
|
|
|
<p>
|
|
|
|
Waiting for approval need not be wasted time. This is a good time to
|
|
start working on something else, clean up unused workspaces, etc. In
|
|
particular, you might want to run long running tests that have not yet
|
|
been run on the your change (different grades, different architectures,
|
|
optimisation levels, etc).
|
|
|
|
<p>
|
|
|
|
The reviewer(s) should reply, indicate any problems that need to be
|
|
corrected, and whether the change can be committed yet. Design issues
|
|
may need to be fully justified before you commit. You may need to fix
|
|
the problems, then go through another iteration of the review process,
|
|
or you may be able to just fix a few small problems, then commit.
|
|
|
|
<p>
|
|
|
|
<h2> Committing </h2>
|
|
|
|
If you have added any new files or directories, then before committing
|
|
you must check the group-id and permissions of the newly created files
|
|
or directories in the CVS repository. Files should be readable by
|
|
group mercury and directories should be both readable and writable by
|
|
group mercury. (Setting of permissions will be enforced by the
|
|
pre-commit check script `CVSROOT/check.pl'.)
|
|
|
|
<p>
|
|
|
|
Use the log message you prepared for the review when committing.
|
|
|
|
<p>
|
|
|
|
<h2> Exceptions: Commit before review </h2>
|
|
|
|
<p>
|
|
|
|
The only time changes should be committed before being reviewed is when they
|
|
satisfy all of the following conditions:
|
|
|
|
<ul>
|
|
<li> (a) the change is simple
|
|
|
|
<li> (b) you are absolutely sure the change will not introduce bugs
|
|
|
|
<li> (c) you are sure that the change will pass review with only
|
|
trivial corrections (spelling errors in comments, etc.)
|
|
|
|
<li> (d) there are no new design decisions or changes to previous
|
|
design decisions in your change (the status quo should
|
|
be the default; you must convince the reviewer(s) of
|
|
the validity of your design decisions before the code
|
|
is committed).
|
|
|
|
<li> (e) you will be around the next day or two to fix the bugs
|
|
that you were sure could never happen
|
|
|
|
<li> (f) committing it now will make life significantly easier
|
|
for you or someone else in the group
|
|
</ol>
|
|
|
|
<p>
|
|
|
|
If the compiler is already broken (i.e. it doesn't pass it's nightly
|
|
tests), and your change is a bug fix, then it's not so important to be
|
|
absolutely sure that your change won't introduce bugs. You should
|
|
still be careful, though. Make sure you review the diffs yourself.
|
|
|
|
<p>
|
|
|
|
Similarly, if the code you are modifying is a presently unused part of
|
|
code - for example a new feature that nobody else is using, that is
|
|
switchable, and is switched off by default, or a new tool, or an `under
|
|
development' webpage that is not linked to by other webpages yet, the
|
|
criteria are a bit looser. Don't use this one too often - only for
|
|
small changes. You don't want to go a long way down the wrong track
|
|
with your new feature, before finding there's a much better way.
|
|
|
|
<p>
|
|
|
|
If these conditions are satisfied, then there shouldn't be any problem
|
|
with mailing the diff, then committing, then fixing any problems that
|
|
come up afterwards, provided you're pretty sure everything will be okay.
|
|
This is particularly true if others are waiting for your work.
|
|
|
|
<p>
|
|
|
|
Usually, a change that has already been reviewed falls into this
|
|
category, provided you have addressed the reviewers comments, and
|
|
there are no disputes over design decisions. If the reviewer has
|
|
specifically asked for another review, or there were a large number of
|
|
comments at the review, you should not commit before a second review.
|
|
|
|
<p>
|
|
|
|
If you are going to commit before the review, use the subject line:<br>
|
|
"diff: <short description of change>".
|
|
|
|
<h2> Exceptions: No review </h2>
|
|
|
|
<p>
|
|
|
|
The only time changes should be committed without review by a second
|
|
person is when they satisfy all of the following conditions:
|
|
|
|
<ul>
|
|
<li> (a) it is a very small diff that is obviously correct <br>
|
|
eg: fix typographic errors <br>
|
|
fix syntax errors you accidently introduced <br>
|
|
fix spelling of people's names <br> <p>
|
|
|
|
These usually don't need to be reviewed by a second
|
|
person. Make sure that you review your own changes,
|
|
though. Also make sure your log message is more
|
|
informative than "fixed a typo", try "s/foo/bar" or
|
|
something so that if you did make a change that people
|
|
don't approve of, at least it's seen quickly.
|
|
|
|
<li> (b) it is not going to be publically visible <br>
|
|
eg: Web pages, documentation, library, man pages. <p>
|
|
|
|
Changes to publically visible stuff should always be
|
|
reviewed. It's just too easy to make spelling errors,
|
|
write incorrect information, commit libel, etc. This
|
|
stuff reflects on the whole group, so it shouldn't be
|
|
ignored.
|
|
</ul>
|
|
|
|
If your change falls into this category, you should still send the
|
|
diff and log message to mercury-developers, but use the subject line:<br>
|
|
"trivial diff: <short description of change>".
|
|
|
|
|
|
<hr>
|
|
<!-------------------------->
|
|
|
|
Last update was $Date: 1997-12-05 02:05:50 $ by $Author: trd $@cs.mu.oz.au. <br>
|
|
</body>
|
|
</html>
|