summaryrefslogtreecommitdiff
path: root/doc/HACKING/HowToReview.md
diff options
context:
space:
mode:
authorNick Mathewson <nickm@torproject.org>2015-10-29 10:30:27 -0400
committerNick Mathewson <nickm@torproject.org>2015-10-29 10:31:38 -0400
commit92a6c578d7038556af391de23081d314091dea32 (patch)
treebc50954a35badc7c4c0b87ee35ab9c1570a13fad /doc/HACKING/HowToReview.md
parente5976482a39ba28efa64764cdb19e310c84aec21 (diff)
downloadtor-92a6c578d7038556af391de23081d314091dea32.tar.gz
tor-92a6c578d7038556af391de23081d314091dea32.zip
hacking is now markdown
Not good markdown, mind you.
Diffstat (limited to 'doc/HACKING/HowToReview.md')
-rw-r--r--doc/HACKING/HowToReview.md85
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..d6b40db937
--- /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.)