1# How to do code reviews for curl 2 3Anyone and everyone is encouraged and welcome to review code submissions in 4curl. This is a guide on what to check for and how to perform a successful 5code review. 6 7## All submissions should get reviewed 8 9All pull requests and patches submitted to the project should be reviewed by 10at least one experienced curl maintainer before that code is accepted and 11merged. 12 13## Let the tools and tests take the first rounds 14 15On initial pull requests, let the tools and tests do their job first and then 16start out by helping the submitter understand the test failures and tool 17alerts. 18 19## How to provide feedback to author 20 21Be nice. Ask questions. Provide examples or suggestions of improvements. 22Assume the best intentions. Remember language barriers. 23 24All first-time contributors can become regulars. Let's help them go there. 25 26## Is this a change we want? 27 28If this is not a change that seems to be aligned with the project's path 29forward and as such cannot be accepted, inform the author about this sooner 30rather than later. Do it gently and explain why and possibly what could be 31done to make it more acceptable. 32 33## API/ABI stability or changed behavior 34 35Changing the API and the ABI may be fine in a change but it needs to be done 36deliberately and carefully. If not, a reviewer must help the author to realize 37the mistake. 38 39curl and libcurl are similarly strict on not modifying existing behavior. API 40and ABI stability is not enough, the behavior should also remain intact as far 41as possible. 42 43## Code style 44 45Most code style nits are detected by checksrc but not all. Only leave remarks 46on style deviation once checksrc does not find anymore. 47 48Minor nits from fresh submitters can also be handled by the maintainer when 49merging, in case it seems like the submitter is not clear on what to do. We 50want to make the process fun and exciting for new contributors. 51 52## Encourage consistency 53 54Make sure new code is written in a similar style as existing code. Naming, 55logic, conditions, etc. 56 57## Are pointers always non-NULL? 58 59If a function or code rely on pointers being non-NULL, take an extra look if 60that seems to be a fair assessment. 61 62## Asserts 63 64Conditions that should never be false can be verified with `DEBUGASSERT()` 65calls to get caught in tests and debugging easier, while not having an impact 66on final or release builds. 67 68## Memory allocation 69 70Can the mallocs be avoided? Do not introduce mallocs in any hot paths. If 71there are (new) mallocs, can they be combined into fewer calls? 72 73Are all allocations handled in error paths to avoid leaks and crashes? 74 75## Thread-safety 76 77We do not like static variables as they break thread-safety and prevent 78functions from being reentrant. 79 80## Should features be `#ifdef`ed? 81 82Features and functionality may not be present everywhere and should therefore 83be `#ifdef`ed. Additionally, some features should be possible to switch on/off 84in the build. 85 86Write `#ifdef`s to be as little of a "maze" as possible. 87 88## Does it look portable enough? 89 90curl runs "everywhere". Does the code take a reasonable stance and enough 91precautions to be possible to build and run on most platforms? 92 93Remember that we live by C89 restrictions. 94 95## Tests and testability 96 97New features should be added in conjunction with one or more test cases. 98Ideally, functions should also be written so that unit tests can be done to 99test individual functions. 100 101## Documentation 102 103New features or changes to existing functionality **must** be accompanied by 104updated documentation. Submitting that in a separate follow-up pull request is 105not OK. A code review must also verify that the submitted documentation update 106matches the code submission. 107 108English is not everyone's first language, be mindful of this and help the 109submitter improve the text if it needs a rewrite to read better. 110 111## Code should not be hard to understand 112 113Source code should be written to maximize readability and be easy to 114understand. 115 116## Functions should not be large 117 118A single function should never be large as that makes it hard to follow and 119understand all the exit points and state changes. Some existing functions in 120curl certainly violate this ground rule but when reviewing new code we should 121propose splitting into smaller functions. 122 123## Duplication is evil 124 125Anything that looks like duplicated code is a red flag. Anything that seems to 126introduce code that we *should* already have or provide needs a closer check. 127 128## Sensitive data 129 130When credentials are involved, take an extra look at what happens with this 131data. Where it comes from and where it goes. 132 133## Variable types differ 134 135`size_t` is not a fixed size. `time_t` can be signed or unsigned and have 136different sizes. Relying on variable sizes is a red flag. 137 138Also remember that endianness and >= 32 bit accesses to unaligned addresses 139are problematic areas. 140 141## Integer overflows 142 143Be careful about integer overflows. Some variable types can be either 32 bit 144or 64 bit. Integer overflows must be detected and acted on *before* they 145happen. 146 147## Dangerous use of functions 148 149Maybe use of `realloc()` should rather use the dynbuf functions? 150 151Do not allow new code that grows buffers without using dynbuf. 152 153Use of C functions that rely on a terminating zero must only be used on data 154that really do have a null-terminating zero. 155 156## Dangerous "data styles" 157 158Make extra precautions and verify that memory buffers that need a terminating 159zero always have exactly that. Buffers *without* a null-terminator must not be 160used as input to string functions. 161 162# Commit messages 163 164Tightly coupled with a code review is making sure that the commit message is 165good. It is the responsibility of the person who merges the code to make sure 166that the commit message follows our standard (detailed in the 167[CONTRIBUTE](CONTRIBUTE.md) document). This includes making sure the PR 168identifies related issues and giving credit to reporters and helpers. 169