mirror of
https://gitlab.torproject.org/tpo/core/tor.git
synced 2024-11-27 22:03:31 +01:00
Actually add HowToReview.txt
This commit is contained in:
parent
609c1e8870
commit
2929986049
85
doc/HACKING/HowToReview.txt
Normal file
85
doc/HACKING/HowToReview.txt
Normal file
@ -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.)
|
Loading…
Reference in New Issue
Block a user