How to do a good code review
- Presenter: Sergei Golubchik
- Scribe: Peter Lavin
- Date: 2007-06-07
- Attendees (please register by filling in your name below, and read the instructions for attendees):
- Alexander Nozdrin
- Dmitri Lenev
- AntonyCurtis
- Hartmut Holzgraefe
- Mike Lischke
- GeorgiKodinov
- Andrei Elkin
- Alexey Kopytov
- Eric Herman
- Lenz Grimmer
- Susanne Ebrecht
- Mr or Mrs Next
Contents |
[edit] Presentation
- What's the purpose of a code review ?
- is it a bug ? should it be fixed ? if yes - how ?
- read the bug description first!
- is the proposed solution correct ? in the right place ?
- does it fix the bug or hides it ?
- is it implemented correctly ?
- follows our coding guidelines
- compatible with the future roadmap and the vision
- doesn't duplicate existing code or functionality
- is optimal, efficiently implemented
- most attention to the tight loops
- rarely used code can use relaxed criteria
- properly commented
- all non-trivial parts MUST be commented
- if some place makes me think "why" - it needs a comment
- changeset comment must have bug/wl number and subject
- and short summary of a problem/changes
- properly tested
- the test matches the bug description (check!)
- the test FAILS without a fix
- create a test first, then fix the bug. Then update the result file.
- as a reviewer I assume that the code compiles, without warnings, and is tested
- is it a bug ? should it be fixed ? if yes - how ?
- BUT NOT
- the purpose of a review is NOT to force a developer to write YOUR patch
- developer writes HIS patch
- been there, done that
- Rules
- you can only review the code that you're familiar with
- you don't understand - you don't approve!
- educate the coder - if the patch is wrong, explain
- the reviewer is ultimately responsible for the code
- Warning signal
- incompatible change in behavior
- ... or file formats
- ... or index ordering (requires REPAIR)
- fix for a boundary case introduces a noticeable overhead in the common case
- changes many existing test results
- changes more than necessary
- especially in GA code
- incompatible change in behavior
- Remember Common Pitfalls
- Make sure they're documented!
- Example: unintentional ABI changes
- Example: mutex locking order
- Example: changing existing error messages
- See also http://forge.mysql.com/wiki/Code_Review_Process
- Make sure they're documented!
- Tips and Tricks
- diff -p
- rediff (in internals)
- diff folding in vim (in internals)
[edit] Questions
[edit] Questions asked before the session
- (joro) What is the preferred form of a bug fix review ? Should it always be a reply to the commit e-mail ?
- YES.
- (joro) What is the preferred form of a request for a bug fix review ? Forward of the commit e-mail ?
- No. But at least, not for me. Subject line only?
- (joro) What about estimates ? Should the reviewer fill/update the est. completion field in the bug ?
- I don't do that but, yes, if it will take a long time.
- For a short time no estimate is necessary. A big patch should be in the WorkLog.
[edit] Questions asked during the session
- Question: Are there checklists that reviewers could use / should use?
- See http://forge.mysql.com/wiki/Code_Review_Process and this presentation.
- Question: Do reviewers normally use that list already?
- I doubt it :) but I don't know, really.
[edit] Voice recording and other links
- Voice Recording: First OGG File
- Voice Recording: Second OGG File
- Voice Recording: Third OGG File
- Edited IRC log IRC Log File
- Links to other resources: [1]
[edit] Heidelberg Broadcast Sept 20, 2007
Scribe: Peter Lavin
[edit] Remarks
Need an overview of what is going on to determine the action to take.
Description should be an abstract -- don't say too much.
Sometimes you can add assertions for help debugging.
When adding new files make sure they are part of "makedist"
[edit] Questions
- Question:Where do you put an extended bug description?
- Answer:
- Worklog
- bug db
- may go into file command
- internals manual
- in short, there is no universal convention
- but not in the changeset comment
- Question:A good review can take a lot of time. Small patches get a good review large ones don't. What can you do?
- Answer:
- split up your patch
- some version control systems have this built in
- can edit parts of the patch individually
- Question:How many teams follow these rules for code review?
- Answer:
- cluster doesn't
- often the reviewer doesn't have the time to do a proper review