doc/HACKING/HowToReview.md

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