tor/doc/HACKING/HowToReview.md

83 lines
2.1 KiB
Markdown
Raw Normal View History

# How to review a patch
2015-10-22 16:03:04 +02:00
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
2015-10-22 16:03:04 +02:00
(Difficulty: easy)
- Does it compile with `--enable-fatal-warnings`?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- Does `make check-spaces` pass?
2015-10-22 16:03:04 +02:00
- Does `make check-changes` pass?
2015-11-05 15:13:53 +01:00
- Does it have a reasonable amount of tests? Do they pass? Do they leak
memory?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- Do all the new functions, global variables, types, and structure members have
documentation?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- Do all the functions, global variables, types, and structure members with
modified behavior have modified documentation?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- Do all the new torrc options have documentation?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- If this changes Tor's behavior on the wire, is there a design proposal?
2015-10-22 16:03:04 +02:00
- If this changes anything in the code, is there a "changes" file?
2015-10-22 16:03:04 +02:00
## Let's look at the code!
2015-10-22 16:03:04 +02:00
- Does the code conform to `CodingStandards.md`?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- Does the code leak memory?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- If two or more pointers ever point to the same object, is it clear which
pointer "owns" the object?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- Are all allocated resources freed?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- Are all pointers that should be const, const?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- Are `#defines` used for 'magic' numbers?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- Can you understand what the code is trying to do?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- Can you convince yourself that the code really does that?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- Is there duplicated code that could be turned into a function?
2015-10-22 16:03:04 +02:00
## Let's look at the documentation!
2015-10-22 16:03:04 +02:00
2019-10-07 06:48:11 +02:00
- Does the documentation conform to CodingStandards.txt?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- Does it make sense?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- Can you predict what the function will do from its documentation?
2015-10-22 16:03:04 +02:00
## Let's think about security!
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- If there are any arrays, buffers, are you 100% sure that they cannot
overflow?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- If there is any integer math, can it overflow or underflow?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- If there are any allocations, are you sure there are corresponding
deallocations?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- Is there a safer pattern that could be used in any case?
2015-10-22 16:03:04 +02:00
2015-11-05 15:13:53 +01:00
- Have they used one of the Forbidden Functions?
2015-10-22 16:03:04 +02:00
(Also see your favorite secure C programming guides.)