Proposal for Code Reviews

in the Time Frame until the 1.1 Final

Version: 21. 5. 2003

Authors: Joerg Barfurth, Thorsten Behrens, Stephan Bergmann, Martin Hollmichel, Matthias Huetsch, Nikolai Pretzell, Tino Rachui.

Content

Context	1

The Procedure	1

Pitfalls to Avoid	3

The Checklist	3

Further Information	6

Context

Recently came up the requirement, that in all remaining tasks until the OpenOffice.org 1.1 Final, changes are reviewed.

The procedure shall be practical in the pre-release stress, that means: Not consume too much time. On the other hand it shall have some real effect, else even the few time spent in it would be wasted.

Main goal is the avoiding of regression bugs, that could occur during fixing RC- (release candidate) or Final-tasks.

The Procedure

Time

The Task has to be set to „reviewed“ in the comment section (IZ) or set to state “reviewed” (StarOffice intern).

Reviewers

One or two persons, depending on the size of the changes. The author chooses the reviewers. Suggestions for the persons who may review:

peers

developers, that use the changed code

any developer

module/project owner

Steps

It is assumed, the author already fixed the task, compiled and tested it successfully and checked it against the checklist (see below).

The code author gives the bug id as well as a list of the changed code pieces to the reviewers.

The reviewers check the changed code by a checklist (see below).
The checklist is to be used as a reminder. Most rules might be broken in very special (and rare) circumstances.

The reviewers write down all points in the code which seem to be issues according to the checklist. The notes have to be in a form, they can be handed over to the author. Each found issue includes the corresponding tag(s) of the checklist.

The reviewers write down all points in the code, which may not be mentioned in the checklist, but that they found additionally and may lead to bugs. Only issues that may lead to bugs, are to be gathered. Matters of taste, style or layout are not subject of these specific pre-final reviews, even if they are valuable issues otherwise.

Author and reviewers meet.
(To “meet” can be a meeting face-2-face (preferred), but also by phone, IRC or mail.)
The author shortly explains, how she has fixed the bug. The reviewers do not interrupt her (no discussion!).
If the reviewers have questions, they add them to the notes from step 4 and 5. They may silently consider following questions:

Is the bug-fix clear to me, do I really understand it?

Does the bug-fix really and correctly eliminate the cause of the bug?

The author and the reviewers continue to meet. The reviewers tell all points they have written down so far in the previous steps. The author listens and only asks if something is not clear. After that she gets their notes from the reviewers.

The author and the reviewers decide, if this review suffices, or if another meeting is necessary after the author has considered the issues and put changes into the code.

Consequences:

The author considers the arisen issues and may change the code accordingly – or may not. Only the author can decide this in the end. After any changes, she compiles and tests again.

Author and reviewers share their experiences on dev@openoffice.org – what worked, what worked not, what they would change.

Substantial notes should be added to the task (Issuezilla,internal Bugtracker).

The author informs the reviewers about the completion of the issues that arose in the review. If no other meeting was planned (in step 8), now the reviewer can set the task to „reviewed“. (On Issuezilla this may be a comment in the task, in Bugtracker an extra state.)

Pitfalls to Avoid

Such code reviews are not about discussion. The validity of rules in general and for this specific code can be discussed at many occasions, but not during a review. The reviews shall be effective and consume not more time than necessary. Therefore their focus is on finding potential issues and on understanding each other. The final decision about necessary changes is by the author of the code and need not to be discussed in the review, unless the author considers additional research as necessary for her ability to judge an issue.

These reviews just before the 1.1 Final are only about the code changed now for this specific task. The surrounding code may be in a terrible or in a wonderful state, but it seems to work so far, unless there wold be another RC- or 1.1-bug for it. So all checks are to be made for the changed or added parts of the code only.

Don't Hurry. According to experiences of other projects – the typical checking rate is 0,5 to 1 Page (ca. 600 non-commentary words or 20 -30 lines of code) per hour. Experience has shown that at this rate most major issues will be found.

The review will need some time, but this time is spent more efficient, than in fixing later regression bugs.

The Checklist

The checklist is sorted by the locations in code to make it easier for the reviewers to apply it.

Each item on the checklist has a tag. When noting issues (step 4. of the above described process), add the corresponding item tag to each noted issue.

This checklist will be joined with the Coding Guidelines (to be found on tools.openoffice.org) in near future.

General

G_WALL
The changed code does not introduce new warnings during a gcc compile with „dmake wall=true". ( Current warnings cannot always be avoided at the present time. )

G_NNFEATURE
There is no feature implemented, that is not needed in the moment.

G_DBLNAMES
There are no duplicate names (variables, functions, types).

G_ALLOC
All allocation and deletion operations match. They use the same family of functions/operators (new/delete vs. malloc/free) and they match in [] usage for allocated arrays.

G_BOOL
All boolean types are named "bool". The only exception being UNO-interfaces, where sal_Bool is required).

G_INCOMP
Because we are just before a release : There are no incompatible changes in exported classes, except if they are intended to be so.

G_UI
Because we are just before a release: There are no user interface changes, if they have not been especially allowed by management/community.

G_MEMLEAK
The life cycle of all objects, pointers and references is clearly and recognizably defined. There is no chance of undeleted objects on the heap or of pointers pointing into nothing.

G_PATCH
The code has no effect on the Office patch process. Except if this is aligned with management/community.

Classes

C_CHGBEHAVE
Does the new code change invariants of the class, that is: Has the user of the class to know something new to use it correctly, that she needed not to know earlier? If so, is the change documented in the classes documentation?

C_EXPLICIT
All single argument constructors have the keyword "explicit" – or a comment, that they are meant to be used for implicit conversions.

C_CONSTFUNC
All member functions that don't change an object's visible state, are const.

C_VIRTUALDEST
All virtual classes have a destructor which is either virtual or protected.

C_COPY
All classes either support copying (may be implicitly by compiler provided versions of operator=() and copy constructor) or they suppress it (by making the compiler throw an error, if a copy is tried).

Function Prototypes

FP_CONSTREF
All in-parameters per reference in any function are const.

FP_OUTPARAM
All out-, inout- and pass_ownership-parameters can be recognized easily and unmistakably.

FP_EXCEPT
Are all exception specifications consistent? (Generally exception specifications should be avoided.)

Special Function Bodies

constructors

FB_CTOR_INIT
All data members of all classes/structs are initialized in every constructor.

FB_CTOR_SEQUENCE
All data members of a class in every constructor are initialized in order of declaration.

FB_CTOR_VIRTUAL
There are no virtual functions called in constructors.

operator=()

FB_OP=_SELF
All operator=() work also in case of self-assignment.

destructors

FB_DTOR_EXCEPT
There is no destructor, which throws an exception.

FB_DTOR_VIRTUAL
There are no virtual functions called in destructors.

All Function Bodies

FB_CLARITY
The code is understandable. Especially difficult places are clearly commented.

FB_CHGBEHAVE
Does the new code change the contract of the function, that is: Has the user of the function to know something new to use it correctly, that she needed not to know earlier? If so, is the change documented in the functions prototype-documentation?

FB_CONST
All automatic variables that will not change are const.

FB_INIT
All automatic variables are initialized at declaration.

FB_FOR
Each variable declared in a for-statement is never used outside that for-statement (that is necessary because of the mismatching concurrent rules for the for scope in different compiler versions).

FB_SWITCHDEF
All switch statements have a default case - or a comment why not.

FB_SWITCHBREAK
All switch statements have a break in every case – or a comment why not.

FB_RANGE
All ranges include lower bound, exclude upper bound. Very special algorithms might differ, but should document that.

FB_PTR
All dereferenced pointers are checked, or they are guaranteed to work by invariants.

FB_HACK
There are no hacks, but valid solutions. If a hack is unavoidable, it is clearly commented in the code.

FB_64
The code is 64-bit clean (for example casts from long to sal_Int32 or between pointer and numbers).

Global File Scope

GF_COMPLEX
There are no complicated initialized global variables (especially no String-, rtl::OUString-Arrays).

GF_INIT
All global variables are initialized at declaration.

GF_CONST
All global variables that will not change are const.

Further Information

Tom Gil, “Software Inspections”, ISBN: 0-201-63181-4

6

Nikolai Pretzell

2003-05-20T17:34:11

Martin Hollmichel

Nikolai Pretzell