mirror of
https://github.com/Mercury-Language/mercury.git
synced 2025-12-16 14:25:56 +00:00
30050aa2fa
Estimated hours taken: 0.1
Branches: main
mercury/compiler/notes/reviews.html:
Fix a couple of HTML errors:
- Replace angled brackets in text with < and >
- Replace an </ol> with a </ul> so it correctly ends the
corresponding begin unordered list tag.
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-reviews@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-reviews@cs.mu.oz.au.
|
|
|
|
<p>
|
|
|
|
The reasons for posting to mercury-reviews 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
|
|
</ul>
|
|
|
|
<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-reviews, but use the subject line:<br>
|
|
"trivial diff: <short description of change>".
|
|
|
|
|
|
<hr>
|
|
<!-------------------------->
|
|
|
|
Last update was $Date: 2003-01-15 08:20:13 $ by $Author: mjwybrow $@cs.mu.oz.au. <br>
|
|
</body>
|
|
</html>
|