Difference between revisions of "CodeReview"

From SambaWiki
Jump to: navigation, search
(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 you would review
+
reviews his/her own patches in the same way one would review
 
patches from others!
 
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++".
+
* 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++".
+
* 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 s "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 change possibly change the blackbox behavior of a function.
+
* 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're maybe done. This happens most of the time and most of the time careful_mode is also 0. And the costs are not too high.
+
* 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
+
* 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!