tor/doc/HACKING/HowToReview.md

How to review a patch
=====================

Some folks have said that they'd like to review patches more often, but they
don't know how.

So, here are a bunch of things to check for when reviewing a patch!

Note that if you can't do every one of these, that doesn't mean you can't do
a good review!  Just make it clear what you checked for and what you didn't.


Top-level smell-checks
----------------------

(Difficulty: easy)

- Does it compile with `--enable-fatal-warnings`?

- Does `make check-spaces` pass?

- Does `make check-changes` pass?

- Does it have a reasonable amount of tests?  Do they pass?  Do they leak
  memory?

- Do all the new functions, global variables, types, and structure members have
 documentation?

- Do all the functions, global variables, types, and structure members with
  modified behavior have modified documentation?

- Do all the new torrc options have documentation?

- If this changes Tor's behavior on the wire, is there a design proposal?

- If this changes anything in the code, is there a "changes" file?


Let's look at the code!
-----------------------

- Does the code conform to CodingStandards.txt?

- Does the code leak memory?

- If two or more pointers ever point to the same object, is it clear which
  pointer "owns" the object?

- Are all allocated resources freed?

- Are all pointers that should be const, const?

- Are `#defines` used for 'magic' numbers?

- Can you understand what the code is trying to do?

- Can you convince yourself that the code really does that?

- Is there duplicated code that could be turned into a function?


Let's look at the documentation!
--------------------------------

- Does the documentation confirm to CodingStandards.txt?

- Does it make sense?

- Can you predict what the function will do from its documentation?


Let's think about security!
---------------------------

- If there are any arrays, buffers, are you 100% sure that they cannot
  overflow?

- If there is any integer math, can it overflow or underflow?

- If there are any allocations, are you sure there are corresponding
  deallocations?

- Is there a safer pattern that could be used in any case?

- Have they used one of the Forbidden Functions?

(Also see your favorite secure C programming guides.)