CODE_REVIEW.md: how to do code reviews in curl

Assisted-by: Daniel Gustafsson
Assisted-by: Rich Salz
Assisted-by: Hugo van Kemenade
Assisted-by: James Fuller
Assisted-by: Marc Hörsken
Assisted-by: Jay Satiro

Closes #5555

docs/CODE_REVIEW.md

@@ -0,0 +1,168 @@
+# How to do code reviews for curl
+
+Anyone and everyone is encouraged and welcome to review code submissions in
+curl. This is a guide on what to check for and how to perform a successful
+code review.
+
+## All submissions should get reviewed
+
+All pull requests and patches submitted to the project should be reviewed by
+at least one experienced curl maintainer before that code is accepted and
+merged.
+
+## Let the tools and tests take the first rounds
+
+On initial pull requests, let the tools and tests do their job first and then
+start out by helping the submitter understand the test failures and tool
+alerts.
+
+## How to provide feedback to author
+
+Be nice. Ask questions. Provide examples or suggestions of improvements.
+Assume best intentions. Remember language barriers.
+
+All first-time contributors can become regulars. Let's help them go there.
+
+## Is this a change we want?
+
+If this is not a change that seems to be aligned with the project's path
+forward and as such cannot be accepted, inform the author about this sooner
+rather than later. Do it gently and explain why and possibly what could be
+done to make it more acceptable.
+
+## API/ABI stability or changed behavior
+
+Changing the API and the ABI may be fine in a change but it needs to be done
+deliberately and carefully. If not, a reviewer must help the author to realize
+the mistake.
+
+curl and libcurl are similarly very strict on not modifying existing
+behavior. API and ABI stability is not enough, the behavior should also remain
+intact as far as possible.
+
+## Code style
+
+Most code style nits are detected by checksrc but not all. Only leave remarks
+on style deviation once checksrc doesn't find any more.
+
+Minor nits from fresh submitters can also be handled by the maintainer when
+merging, in case it seems like the submitter isn't clear on what to do. We
+want to make the process fun and exciting for new contributors.
+
+## Encourage consistency
+
+Make sure new code is written in a similar style as existing code. Naming,
+logic, conditions, etc.
+
+## Are pointers always non-NULL?
+
+If a function or code rely on pointers being non-NULL, take an extra look if
+that seems to be a fair assessment.
+
+## Asserts
+
+Conditions that should never be false can be verified with `DEBUGASSERT()`
+calls to get caught in tests and debugging easier, while not having an impact
+on final or release builds.
+
+## Memory allocation
+
+Can the mallocs be avoided? Do not introduce mallocs in any hot paths. If
+there are (new) mallocs, can they be combined into fewer calls?
+
+Are all allocations handled in errorpaths to avoid leaks and crashes?
+
+## Thread-safety
+
+We do not like static variables as they break thread-safety and prevent
+functions from being reentrant.
+
+## Should features be `#ifdef`ed?
+
+Features and functionality may not be present everywhere and should therefore
+be `#ifdef`ed. Additionally, some features should be possible to switch on/off
+in the build.
+
+Write `#ifdef`s to be as little of a "maze" as possible.
+
+## Does it look portable enough?
+
+curl runs "everywhere". Does the code take a reasonable stance and enough
+precautions to be possible to build and run on most platforms?
+
+Remember that we live by C89 restrictions.
+
+## Tests and testability
+
+New features should be added in conjunction with one or more test cases.
+Ideally, functions should also be written so that unit tests can be done to
+test individual functions.
+
+## Documentation
+
+New features or changes to existing functionality **must** be accompanied with
+updated documentation. Submitting that in a separate follow-up pull request is
+not OK. A code review must also verify that the submitted documentation update
+matches the code submission.
+
+English isn't everyone's first language, be mindful of this and help the
+submitter improve the text if it needs a rewrite to read better.
+
+## Code shouldn't be hard to understand
+
+Source code should be written to maximize readability and be easy to
+understand.
+
+## Functions shouldn't be large
+
+A single function should never be large as that makes it hard to follow and
+understand all the exit points and state changes. Some existing functions in
+curl certainly violate this ground rule but when reviewing new code we should
+propose splitting into smaller functions.
+
+## Duplication is evil
+
+Anything that looks like duplicated code is a red flag. Anything that seems to
+introduce code that we *should* already have or provide needs a closer check.
+
+## Sensitive data
+
+When credentials are involved, take an extra look at what happens with this
+data. Where it comes from and where it goes.
+
+## Variable types differ
+
+`size_t` is not a fixed size. `time_t` can be signed or unsigned and have
+different sizes. Relying on variable sizes is a red flag.
+
+Also remember that endianness and >= 32 bit accesses to unaligned addresses
+are problematic areas.
+
+## Integer overflows
+
+Be careful about integer overflows. Some variable types can be either 32 bit
+or 64 bit. Integer overflows must be detected and acted on *before* they
+happen.
+
+## Dangerous use of functions
+
+Maybe use of `realloc()` should rather use the dynbuf functions?
+
+Do not allow new code that grows buffers without using dynbuf.
+
+Use of C functions that rely on a terminating zero must only be used on data
+that really do have a zero terminating zero.
+
+## Dangerous "data styles"
+
+Make extra precautions and verify that memory buffers that need a terminating
+zero always have exactly that. Buffers *without* a zero terminator must not be
+used as input to string functions.
+
+# Commit messages
+
+Tightly coupled with a code review is making sure that the commit message is
+good. It is the responsibilitiy of the person who merges the code to make sure
+that the commit message follows our standard (detailed in the
+[CONTRIBUTE.md](CONTRIBUTE.md) document). This includes making sure the PR
+identifies related issues and giving credit to reporters and helpers.

docs/Makefile.am

@@ -50,6 +50,7 @@ EXTRA_DIST =                                    \
  CIPHERS.md                                     \
  CMakeLists.txt                                 \
  CODE_OF_CONDUCT.md                             \
+ CODE_REVIEW.md                                 \
  CODE_STYLE.md                                  \
  CONTRIBUTE.md                                  \
  CURL-DISABLE.md                                \