diff options
Diffstat (limited to 'doc/HACKING/HowToReview.md')
-rw-r--r-- | doc/HACKING/HowToReview.md | 85 |
1 files changed, 85 insertions, 0 deletions
diff --git a/doc/HACKING/HowToReview.md b/doc/HACKING/HowToReview.md new file mode 100644 index 0000000000..de7891c923 --- /dev/null +++ b/doc/HACKING/HowToReview.md @@ -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 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.) |