Difference between revisions of "CodeReview"
From SambaWiki
(initial page) |
m (some typo fixes) |
||
| Line 3: | Line 3: | ||
I think everybody can learn how to do good code review | I think everybody can learn how to do good code review | ||
and I think the most important thing is that everybody | and I think the most important thing is that everybody | ||
| − | reviews his/her own patches in the same way | + | reviews his/her own patches in the same way one would review |
patches from others! | patches from others! | ||
| − | * First read the commit message | + | * First read the commit message. Does that give you the impression the change is in anyway complex? If so do, a "careful_mode++". |
| − | * Read the diffstat | + | * Read the diffstat. Does this change touches any critical areas of code? If so, do a "careful_mode++". |
| − | * First read the patch hunk by hunk and watch for Coding-Style issues. If there strange things do | + | * First read the patch hunk by hunk and watch for Coding-Style issues. If there are strange things, do a "careful_mode++". |
| − | * Read the patch again as whole, watch for changes which could | + | * Read the patch again as whole, watch for changes which could possibly change the blackbox behavior of a function. |
| − | * Depending on the "careful_mode" you should try to fully understand the change. | + | * Depending on the "careful_mode", you should try to fully understand the change. |
* Note: Here it's sometimes useful to use the -U<contextlines> feature of git show/diff, so that you see the changes in the full context of the whole function. | * Note: Here it's sometimes useful to use the -U<contextlines> feature of git show/diff, so that you see the changes in the full context of the whole function. | ||
| − | * If you found a change in the behavior set "very_careful_mode = true". | + | * If you found a change in the behavior, set "very_careful_mode = true". |
| − | * Ask yourself why a change is needed! If you don't see why it's needed set "very_careful_mode = true". | + | * Ask yourself why a change is needed! If you don't see why it's needed, set "very_careful_mode = true". |
| − | * If you find things you don't understand set "verfy_careful_mode = true". | + | * If you find things you don't understand, set "verfy_careful_mode = true". |
| − | * If very_careful_mode != true you | + | * If very_careful_mode != true you may be done. This happens most of the time and most of the time careful_mode is also 0. And the costs are not too high. |
* Maybe ask other people for review. Trigger discussions on the samba-technical@samba.org mailing list. | * Maybe ask other people for review. Trigger discussions on the samba-technical@samba.org mailing list. | ||
| − | * Read the related sources | + | * Read the related sources. |
| − | * | + | * Use git grep to find callers of a function. |
| − | * Do manual runtime tests | + | * Do manual runtime tests. |
== Review yourself == | == Review yourself == | ||
Revision as of 08:49, 17 October 2012
Code review strangies
I think everybody can learn how to do good code review and I think the most important thing is that everybody reviews his/her own patches in the same way one would review patches from others!
- First read the commit message. Does that give you the impression the change is in anyway complex? If so do, a "careful_mode++".
- Read the diffstat. Does this change touches any critical areas of code? If so, do a "careful_mode++".
- First read the patch hunk by hunk and watch for Coding-Style issues. If there are strange things, do a "careful_mode++".
- Read the patch again as whole, watch for changes which could possibly change the blackbox behavior of a function.
- Depending on the "careful_mode", you should try to fully understand the change.
- Note: Here it's sometimes useful to use the -U<contextlines> feature of git show/diff, so that you see the changes in the full context of the whole function.
- If you found a change in the behavior, set "very_careful_mode = true".
- Ask yourself why a change is needed! If you don't see why it's needed, set "very_careful_mode = true".
- If you find things you don't understand, set "verfy_careful_mode = true".
- If very_careful_mode != true you may be done. This happens most of the time and most of the time careful_mode is also 0. And the costs are not too high.
- Maybe ask other people for review. Trigger discussions on the samba-technical@samba.org mailing list.
- Read the related sources.
- Use git grep to find callers of a function.
- Do manual runtime tests.
Review yourself
I think the most important thing is that everybody reviews his/her own patches in the same way you would review patches from others!