| 12345678910111213141516171819202122232425262728293031323334353637383940414243444546474849505152535455565758596061626364656667686970717273747576777879808182838485 |
- 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.)
|
