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