diff options
Diffstat (limited to 'doc/developer-notes.md')
| -rw-r--r-- | doc/developer-notes.md | 347 |
1 files changed, 329 insertions, 18 deletions
diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 30d365fb7..2cea300b8 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -1,17 +1,25 @@ -Coding -==================== +Developer Notes +=============== Various coding styles have been used during the history of the codebase, and the result is not very consistent. However, we're now trying to converge to a single style, so please use it in new code. Old code will be converted -gradually. -- Basic rules specified in src/.clang-format. Use a recent clang-format-3.5 to format automatically. +gradually and you are encouraged to use the provided +[clang-format-diff script](/contrib/devtools/README.md#clang-format-diffpy) +to clean up the patch automatically before submitting a pull request. + +- Basic rules specified in [src/.clang-format](/src/.clang-format). - Braces on new lines for namespaces, classes, functions, methods. - Braces on the same line for everything else. - 4 space indentation (no tabs) for every block except namespaces. - - No indentation for public/protected/private or for namespaces. + - No indentation for `public`/`protected`/`private` or for `namespace`. - No extra spaces inside parenthesis; don't do ( this ) - - No space after function names; one space after if, for and while. + - No space after function names; one space after `if`, `for` and `while`. + - If an `if` only has a single-statement then-clause, it can appear + on the same line as the if, without braces. In every other case, + braces are required, and the then and else clauses must appear + correctly indented on a new line. + - `++i` is preferred over `i++`. Block style example: ```c++ @@ -19,14 +27,18 @@ namespace foo { class Class { - bool Function(char* psz, int n) + bool Function(const std::string& s, int n) { // Comment summarising what this section of code does - for (int i = 0; i < n; i++) { + for (int i = 0; i < n; ++i) { // When something fails, return early - if (!Something()) - return false; + if (!Something()) return false; ... + if (SomethingElse()) { + DoMore(); + } else { + DoLess(); + } } // Success return is usually at the end @@ -57,7 +69,7 @@ As Doxygen recognizes the comments by the delimiters (`/**` and `*/` in this cas To describe a class use the same construct above the class definition: ```c++ -/** +/** * Alerts are for notifying old versions if they become too obsolete and * need to upgrade. The message is displayed in the status bar. * @see GetWarnings() @@ -71,6 +83,12 @@ To describe a member or variable use: int var; //!< Detailed description after the member ``` +or +```cpp +//! Description before the member +int var; +``` + Also OK: ```c++ /// @@ -102,7 +120,7 @@ CXXFLAGS="-g -ggdb -O0" or whatever debug flags you need. If the code is behaving strangely, take a look in the debug.log file in the data directory; error and debugging messages are written there. -The -debug=... command-line option controls debugging; running with just -debug will turn +The -debug=... command-line option controls debugging; running with just -debug or -debug=1 will turn on all categories (and give you a very large debug.log file). The Qt code routes qDebug() output to debug.log under category "qt": run with -debug=qt @@ -172,15 +190,308 @@ Threads - Shutdown : Does an orderly shutdown of everything. -Pull Request Terminology +Ignoring IDE/editor files +-------------------------- + +In closed-source environments in which everyone uses the same IDE it is common +to add temporary files it produces to the project-wide `.gitignore` file. + +However, in open source software such as Bitcoin Core, where everyone uses +their own editors/IDE/tools, it is less common. Only you know what files your +editor produces and this may change from version to version. The canonical way +to do this is thus to create your local gitignore. Add this to `~/.gitconfig`: + +``` +[core] + excludesfile = /home/.../.gitignore_global +``` + +(alternatively, type the command `git config --global core.excludesfile ~/.gitignore_global` +on a terminal) + +Then put your favourite tool's temporary filenames in that file, e.g. +``` +# NetBeans +nbproject/ +``` + +Another option is to create a per-repository excludes file `.git/info/exclude`. +These are not committed but apply only to one repository. + +If a set of tools is used by the build system or scripts the repository (for +example, lcov) it is perfectly acceptable to add its files to `.gitignore` +and commit them. + +Development guidelines +============================ + +A few non-style-related recommendations for developers, as well as points to +pay attention to for reviewers of Bitcoin Core code. + +General Bitcoin Core +---------------------- + +- New features should be exposed on RPC first, then can be made available in the GUI + + - *Rationale*: RPC allows for better automatic testing. The test suite for + the GUI is very limited + +- Make sure pull requests pass Travis CI before merging + + - *Rationale*: Makes sure that they pass thorough testing, and that the tester will keep passing + on the master branch. Otherwise all new pull requests will start failing the tests, resulting in + confusion and mayhem + + - *Explanation*: If the test suite is to be updated for a change, this has to + be done first + +Wallet +------- + +- Make sure that no crashes happen with run-time option `-disablewallet`. + + - *Rationale*: In RPC code that conditionally uses the wallet (such as + `validateaddress`) it is easy to forget that global pointer `pwalletMain` + can be NULL. See `qa/rpc-tests/disablewallet.py` for functional tests + exercising the API with `-disablewallet` + +- Include `db_cxx.h` (BerkeleyDB header) only when `ENABLE_WALLET` is set + + - *Rationale*: Otherwise compilation of the disable-wallet build will fail in environments without BerkeleyDB + +General C++ +------------- + +- Assertions should not have side-effects + + - *Rationale*: Even though the source code is set to to refuse to compile + with assertions disabled, having side-effects in assertions is unexpected and + makes the code harder to understand + +- If you use the `.h`, you must link the `.cpp` + + - *Rationale*: Include files define the interface for the code in implementation files. Including one but + not linking the other is confusing. Please avoid that. Moving functions from + the `.h` to the `.cpp` should not result in build errors + +- Use the RAII (Resource Acquisition Is Initialization) paradigm where possible. For example by using + `unique_ptr` for allocations in a function. + + - *Rationale*: This avoids memory and resource leaks, and ensures exception safety + +C++ data structures +-------------------- + +- Never use the `std::map []` syntax when reading from a map, but instead use `.find()` + + - *Rationale*: `[]` does an insert (of the default element) if the item doesn't + exist in the map yet. This has resulted in memory leaks in the past, as well as + race conditions (expecting read-read behavior). Using `[]` is fine for *writing* to a map + +- Do not compare an iterator from one data structure with an iterator of + another data structure (even if of the same type) + + - *Rationale*: Behavior is undefined. In C++ parlor this means "may reformat + the universe", in practice this has resulted in at least one hard-to-debug crash bug + +- Watch out for out-of-bounds vector access. `&vch[vch.size()]` is illegal, + including `&vch[0]` for an empty vector. Use `vch.data()` and `vch.data() + + vch.size()` instead. + +- Vector bounds checking is only enabled in debug mode. Do not rely on it + +- Make sure that constructors initialize all fields. If this is skipped for a + good reason (i.e., optimization on the critical path), add an explicit + comment about this + + - *Rationale*: Ensure determinism by avoiding accidental use of uninitialized + values. Also, static analyzers balk about this. + +- Use explicitly signed or unsigned `char`s, or even better `uint8_t` and + `int8_t`. Do not use bare `char` unless it is to pass to a third-party API. + This type can be signed or unsigned depending on the architecture, which can + lead to interoperability problems or dangerous conditions such as + out-of-bounds array accesses + +- Prefer explicit constructions over implicit ones that rely on 'magical' C++ behavior + + - *Rationale*: Easier to understand what is happening, thus easier to spot mistakes, even for those + that are not language lawyers + +Strings and formatting ------------------------ -Concept ACK - Agree with the idea and overall direction, but have neither reviewed nor tested the code changes. +- Be careful of `LogPrint` versus `LogPrintf`. `LogPrint` takes a `category` argument, `LogPrintf` does not. + + - *Rationale*: Confusion of these can result in runtime exceptions due to + formatting mismatch, and it is easy to get wrong because of subtly similar naming + +- Use `std::string`, avoid C string manipulation functions + + - *Rationale*: C++ string handling is marginally safer, less scope for + buffer overflows and surprises with `\0` characters. Also some C string manipulations + tend to act differently depending on platform, or even the user locale + +- Use `ParseInt32`, `ParseInt64`, `ParseUInt32`, `ParseUInt64`, `ParseDouble` from `utilstrencodings.h` for number parsing + + - *Rationale*: These functions do overflow checking, and avoid pesky locale issues + +- For `strprintf`, `LogPrint`, `LogPrintf` formatting characters don't need size specifiers + + - *Rationale*: Bitcoin Core uses tinyformat, which is type safe. Leave them out to avoid confusion + +Variable names +-------------- + +The shadowing warning (`-Wshadow`) is enabled by default. It prevents issues rising +from using a different variable with the same name. + +Please name variables so that their names do not shadow variables defined in the source code. + +E.g. in member initializers, prepend `_` to the argument name shadowing the +member name: + +```c++ +class AddressBookPage +{ + Mode mode; +} + +AddressBookPage::AddressBookPage(Mode _mode) : + mode(_mode) +... +``` + +When using nested cycles, do not name the inner cycle variable the same as in +upper cycle etc. + + +Threads and synchronization +---------------------------- + +- Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential + deadlocks are introduced. As of 0.12, this is defined by default when + configuring with `--enable-debug` + +- When using `LOCK`/`TRY_LOCK` be aware that the lock exists in the context of + the current scope, so surround the statement and the code that needs the lock + with braces + + OK: + +```c++ +{ + TRY_LOCK(cs_vNodes, lockNodes); + ... +} +``` + + Wrong: + +```c++ +TRY_LOCK(cs_vNodes, lockNodes); +{ + ... +} +``` + +Source code organization +-------------------------- + +- Implementation code should go into the `.cpp` file and not the `.h`, unless necessary due to template usage or + when performance due to inlining is critical + + - *Rationale*: Shorter and simpler header files are easier to read, and reduce compile time + +- Don't import anything into the global namespace (`using namespace ...`). Use + fully specified types such as `std::string`. + + - *Rationale*: Avoids symbol conflicts + +GUI +----- + +- Do not display or manipulate dialogs in model code (classes `*Model`) + + - *Rationale*: Model classes pass through events and data from the core, they + should not interact with the user. That's where View classes come in. The converse also + holds: try to not directly access core data structures from Views. + +Subtrees +---------- + +Several parts of the repository are subtrees of software maintained elsewhere. + +Some of these are maintained by active developers of Bitcoin Core, in which case changes should probably go +directly upstream without being PRed directly against the project. They will be merged back in the next +subtree merge. + +Others are external projects without a tight relationship with our project. Changes to these should also +be sent upstream but bugfixes may also be prudent to PR against Bitcoin Core so that they can be integrated +quickly. Cosmetic changes should be purely taken upstream. + +There is a tool in contrib/devtools/git-subtree-check.sh to check a subtree directory for consistency with +its upstream repository. + +Current subtrees include: + +- src/leveldb + - Upstream at https://github.com/google/leveldb ; Maintained by Google, but open important PRs to Core to avoid delay + +- src/libsecp256k1 + - Upstream at https://github.com/bitcoin-core/secp256k1/ ; actively maintaned by Core contributors. + +- src/crypto/ctaes + - Upstream at https://github.com/bitcoin-core/ctaes ; actively maintained by Core contributors. + +- src/univalue + - Upstream at https://github.com/jgarzik/univalue ; report important PRs to Core to avoid delay. + + +Git and GitHub tips +--------------------- + +- For resolving merge/rebase conflicts, it can be useful to enable diff3 style using + `git config merge.conflictstyle diff3`. Instead of + + <<< + yours + === + theirs + >>> + + you will see + + <<< + yours + ||| + original + === + theirs + >>> + + This may make it much clearer what caused the conflict. In this style, you can often just look + at what changed between *original* and *theirs*, and mechanically apply that to *yours* (or the other way around). + +- When reviewing patches which change indentation in C++ files, use `git diff -w` and `git show -w`. This makes + the diff algorithm ignore whitespace changes. This feature is also available on github.com, by adding `?w=1` + at the end of any URL which shows a diff. + +- When reviewing patches that change symbol names in many places, use `git diff --word-diff`. This will instead + of showing the patch as deleted/added *lines*, show deleted/added *words*. -utACK (untested ACK) - Reviewed and agree with the code changes but haven't actually tested them. +- When reviewing patches that move code around, try using + `git diff --patience commit~:old/file.cpp commit:new/file/name.cpp`, and ignoring everything except the + moved body of code which should show up as neither `+` or `-` lines. In case it was not a pure move, this may + even work when combined with the `-w` or `--word-diff` options described above. -Tested ACK - Reviewed the code changes and have verified the functionality or bug fix. +- When looking at other's pull requests, it may make sense to add the following section to your `.git/config` + file: -ACK - A loose ACK can be confusing. It's best to avoid them unless it's a documentation/comment only change in which case there is nothing to test/verify; therefore the tested/untested distinction is not there. + [remote "upstream-pull"] + fetch = +refs/pull/*:refs/remotes/upstream-pull/* + url = [email protected]:dogecoin/dogecoin.git -NACK - Disagree with the code changes/concept. Should be accompanied by an explanation. + This will add an `upstream-pull` remote to your git repository, which can be fetched using `git fetch --all` + or `git fetch upstream-pull`. Afterwards, you can use `upstream-pull/NUMBER/head` in arguments to `git show`, + `git checkout` and anywhere a commit id would be acceptable to see the changes from pull request NUMBER. |