commit 2929986049780bd28fb39ae8d0a67e703e5193ef Author: Nick Mathewson nickm@torproject.org Date: Thu Oct 22 10:03:04 2015 -0400

Actually add HowToReview.txt --- doc/HACKING/HowToReview.txt | 85 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+)

diff --git a/doc/HACKING/HowToReview.txt b/doc/HACKING/HowToReview.txt new file mode 100644 index 0000000..1380411 --- /dev/null +++ b/doc/HACKING/HowToReview.txt @@ -0,0 +1,85 @@ +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-gcc-warnings? + +Does 'make check-spaces' 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 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? + + + +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.)