doc/HACKING/HowToReview.md

   1 # How to review a patch
   2
   3 Some folks have said that they'd like to review patches more often, but they
   4 don't know how.
   5
   6 So, here are a bunch of things to check for when reviewing a patch!
   7
   8 Note that if you can't do every one of these, that doesn't mean you can't do
   9 a good review!  Just make it clear what you checked for and what you didn't.
  10
  11 ## Top-level smell-checks
  12
  13 (Difficulty: easy)
  14
  15 - Does it compile with `--enable-fatal-warnings`?
  16
  17 - Does `make check-spaces` pass?
  18
  19 - Does `make check-changes` pass?
  20
  21 - Does it have a reasonable amount of tests?  Do they pass?  Do they leak
  22   memory?
  23
  24 - Do all the new functions, global variables, types, and structure members have
  25  documentation?
  26
  27 - Do all the functions, global variables, types, and structure members with
  28   modified behavior have modified documentation?
  29
  30 - Do all the new torrc options have documentation?
  31
  32 - If this changes Tor's behavior on the wire, is there a design proposal?
  33
  34 - If this changes anything in the code, is there a "changes" file?
  35
  36
  37 ## Let's look at the code!
  38
  39 - Does the code conform to `CodingStandards.md`?
  40
  41 - Does the code leak memory?
  42
  43 - If two or more pointers ever point to the same object, is it clear which
  44   pointer "owns" the object?
  45
  46 - Are all allocated resources freed?
  47
  48 - Are all pointers that should be const, const?
  49
  50 - Are `#defines` used for 'magic' numbers?
  51
  52 - Can you understand what the code is trying to do?
  53
  54 - Can you convince yourself that the code really does that?
  55
  56 - Is there duplicated code that could be turned into a function?
  57
  58
  59 ## Let's look at the documentation!
  60
  61 - Does the documentation conform to CodingStandards.txt?
  62
  63 - Does it make sense?
  64
  65 - Can you predict what the function will do from its documentation?
  66
  67
  68 ## Let's think about security!
  69
  70 - If there are any arrays, buffers, are you 100% sure that they cannot
  71   overflow?
  72
  73 - If there is any integer math, can it overflow or underflow?
  74
  75 - If there are any allocations, are you sure there are corresponding
  76   deallocations?
  77
  78 - Is there a safer pattern that could be used in any case?
  79
  80 - Have they used one of the Forbidden Functions?
  81
  82 (Also see your favorite secure C programming guides.)