Merge #16530: doc: Fix grammar and punctuation in developer notes
b2ea20d330
doc: Fix grammar and punctuation in developer notes (Kristian Kramer) Pull request description: This pull request is regarding minor grammar and punctuation errors in the developer notes. There were no modifications to the existing code, only alterations to fix the grammar and punctuation in the text to make the developer notes more understandable and easier to read. ACKs for top commit: fanquake: ACKb2ea20d330
Tree-SHA512: eef990b7e7645b44a1ab0b057f4df35894c307fd23cc861a10d3cc80e7fe7fe8f5b94467f8224cc8a7aaa226f82be3a1f0460a45f3e25e5dab1e1d333c9edbc0
This commit is contained in:
commit
fbe4b7665b
1 changed files with 76 additions and 79 deletions
|
@ -62,7 +62,7 @@ tool to clean up patches automatically before submission.
|
||||||
- Braces on the same line for everything else.
|
- Braces on the same line for everything else.
|
||||||
- 4 space indentation (no tabs) for every block except namespaces.
|
- 4 space indentation (no tabs) for every block except namespaces.
|
||||||
- No indentation for `public`/`protected`/`private` or for `namespace`.
|
- No indentation for `public`/`protected`/`private` or for `namespace`.
|
||||||
- No extra spaces inside parenthesis; don't do ( this )
|
- 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
|
- 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,
|
on the same line as the `if`, without braces. In every other case,
|
||||||
|
@ -72,12 +72,12 @@ tool to clean up patches automatically before submission.
|
||||||
- **Symbol naming conventions**. These are preferred in new code, but are not
|
- **Symbol naming conventions**. These are preferred in new code, but are not
|
||||||
required when doing so would need changes to significant pieces of existing
|
required when doing so would need changes to significant pieces of existing
|
||||||
code.
|
code.
|
||||||
- Variable (including function arguments) and namespace names are all lowercase, and may use `_` to
|
- Variable (including function arguments) and namespace names are all lowercase and may use `_` to
|
||||||
separate words (snake_case).
|
separate words (snake_case).
|
||||||
- Class member variables have a `m_` prefix.
|
- Class member variables have a `m_` prefix.
|
||||||
- Global variables have a `g_` prefix.
|
- Global variables have a `g_` prefix.
|
||||||
- Constant names are all uppercase, and use `_` to separate words.
|
- Constant names are all uppercase, and use `_` to separate words.
|
||||||
- Class names, function names and method names are UpperCamelCase
|
- Class names, function names, and method names are UpperCamelCase
|
||||||
(PascalCase). Do not prefix class names with `C`.
|
(PascalCase). Do not prefix class names with `C`.
|
||||||
- Test suite naming convention: The Boost test suite in file
|
- Test suite naming convention: The Boost test suite in file
|
||||||
`src/test/foo_tests.cpp` should be named `foo_tests`. Test suite names
|
`src/test/foo_tests.cpp` should be named `foo_tests`. Test suite names
|
||||||
|
@ -134,6 +134,7 @@ Bitcoin Core uses [Doxygen](http://www.doxygen.nl/) to generate its official doc
|
||||||
Use Doxygen-compatible comment blocks for functions, methods, and fields.
|
Use Doxygen-compatible comment blocks for functions, methods, and fields.
|
||||||
|
|
||||||
For example, to describe a function use:
|
For example, to describe a function use:
|
||||||
|
|
||||||
```c++
|
```c++
|
||||||
/**
|
/**
|
||||||
* ... text ...
|
* ... text ...
|
||||||
|
@ -143,11 +144,12 @@ For example, to describe a function use:
|
||||||
*/
|
*/
|
||||||
bool function(int arg1, const char *arg2)
|
bool function(int arg1, const char *arg2)
|
||||||
```
|
```
|
||||||
|
|
||||||
A complete list of `@xxx` commands can be found at http://www.doxygen.nl/manual/commands.html.
|
A complete list of `@xxx` commands can be found at http://www.doxygen.nl/manual/commands.html.
|
||||||
As Doxygen recognizes the comments by the delimiters (`/**` and `*/` in this case), you don't
|
As Doxygen recognizes the comments by the delimiters (`/**` and `*/` in this case), you don't
|
||||||
*need* to provide any commands for a comment to be valid; just a description text is fine.
|
*need* to provide any commands for a comment to be valid; just a description text is fine.
|
||||||
|
|
||||||
To describe a class use the same construct above the class definition:
|
To describe a class, use the same construct above the class definition:
|
||||||
```c++
|
```c++
|
||||||
/**
|
/**
|
||||||
* Alerts are for notifying old versions if they become too obsolete and
|
* Alerts are for notifying old versions if they become too obsolete and
|
||||||
|
@ -189,7 +191,7 @@ but the above styles are favored.
|
||||||
|
|
||||||
Documentation can be generated with `make docs` and cleaned up with `make clean-docs`. The resulting files are located in `doc/doxygen/html`; open `index.html` to view the homepage.
|
Documentation can be generated with `make docs` and cleaned up with `make clean-docs`. The resulting files are located in `doc/doxygen/html`; open `index.html` to view the homepage.
|
||||||
|
|
||||||
Before running `make docs`, you will need to install dependencies `doxygen` and `dot`. For example, on MacOS via Homebrew:
|
Before running `make docs`, you will need to install dependencies `doxygen` and `dot`. For example, on macOS via Homebrew:
|
||||||
```
|
```
|
||||||
brew install doxygen --with-graphviz
|
brew install doxygen --with-graphviz
|
||||||
```
|
```
|
||||||
|
@ -231,7 +233,7 @@ that run in `-regtest` mode.
|
||||||
Bitcoin Core is a multi-threaded application, and deadlocks or other
|
Bitcoin Core is a multi-threaded application, and deadlocks or other
|
||||||
multi-threading bugs can be very difficult to track down. The `--enable-debug`
|
multi-threading bugs can be very difficult to track down. The `--enable-debug`
|
||||||
configure option adds `-DDEBUG_LOCKORDER` to the compiler flags. This inserts
|
configure option adds `-DDEBUG_LOCKORDER` to the compiler flags. This inserts
|
||||||
run-time checks to keep track of which locks are held, and adds warnings to the
|
run-time checks to keep track of which locks are held and adds warnings to the
|
||||||
debug.log file if inconsistencies are detected.
|
debug.log file if inconsistencies are detected.
|
||||||
|
|
||||||
### Valgrind suppressions file
|
### Valgrind suppressions file
|
||||||
|
@ -299,7 +301,7 @@ $ perf record \
|
||||||
-p `pgrep bitcoind` -- sleep 60
|
-p `pgrep bitcoind` -- sleep 60
|
||||||
```
|
```
|
||||||
|
|
||||||
You could then analyze the results by running
|
You could then analyze the results by running:
|
||||||
|
|
||||||
```sh
|
```sh
|
||||||
perf report --stdio | c++filt | less
|
perf report --stdio | c++filt | less
|
||||||
|
@ -364,7 +366,7 @@ Additional resources:
|
||||||
Locking/mutex usage notes
|
Locking/mutex usage notes
|
||||||
-------------------------
|
-------------------------
|
||||||
|
|
||||||
The code is multi-threaded, and uses mutexes and the
|
The code is multi-threaded and uses mutexes and the
|
||||||
`LOCK` and `TRY_LOCK` macros to protect data structures.
|
`LOCK` and `TRY_LOCK` macros to protect data structures.
|
||||||
|
|
||||||
Deadlocks due to inconsistent lock ordering (thread 1 locks `cs_main` and then
|
Deadlocks due to inconsistent lock ordering (thread 1 locks `cs_main` and then
|
||||||
|
@ -389,7 +391,7 @@ Threads
|
||||||
|
|
||||||
- ThreadDNSAddressSeed : Loads addresses of peers from the DNS.
|
- ThreadDNSAddressSeed : Loads addresses of peers from the DNS.
|
||||||
|
|
||||||
- ThreadMapPort : Universal plug-and-play startup/shutdown
|
- ThreadMapPort : Universal plug-and-play startup/shutdown.
|
||||||
|
|
||||||
- ThreadSocketHandler : Sends/Receives data from peers on port 8333.
|
- ThreadSocketHandler : Sends/Receives data from peers on port 8333.
|
||||||
|
|
||||||
|
@ -408,7 +410,7 @@ Threads
|
||||||
Ignoring IDE/editor files
|
Ignoring IDE/editor files
|
||||||
--------------------------
|
--------------------------
|
||||||
|
|
||||||
In closed-source environments in which everyone uses the same IDE it is common
|
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.
|
to add temporary files it produces to the project-wide `.gitignore` file.
|
||||||
|
|
||||||
However, in open source software such as Bitcoin Core, where everyone uses
|
However, in open source software such as Bitcoin Core, where everyone uses
|
||||||
|
@ -446,19 +448,19 @@ pay attention to for reviewers of Bitcoin Core code.
|
||||||
General Bitcoin Core
|
General Bitcoin Core
|
||||||
----------------------
|
----------------------
|
||||||
|
|
||||||
- New features should be exposed on RPC first, then can be made available in the GUI
|
- 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
|
- *Rationale*: RPC allows for better automatic testing. The test suite for
|
||||||
the GUI is very limited
|
the GUI is very limited.
|
||||||
|
|
||||||
- Make sure pull requests pass Travis CI before merging
|
- Make sure pull requests pass Travis CI before merging.
|
||||||
|
|
||||||
- *Rationale*: Makes sure that they pass thorough testing, and that the tester will keep passing
|
- *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
|
on the master branch. Otherwise, all new pull requests will start failing the tests, resulting in
|
||||||
confusion and mayhem
|
confusion and mayhem.
|
||||||
|
|
||||||
- *Explanation*: If the test suite is to be updated for a change, this has to
|
- *Explanation*: If the test suite is to be updated for a change, this has to
|
||||||
be done first
|
be done first.
|
||||||
|
|
||||||
Wallet
|
Wallet
|
||||||
-------
|
-------
|
||||||
|
@ -466,13 +468,13 @@ Wallet
|
||||||
- Make sure that no crashes happen with run-time option `-disablewallet`.
|
- Make sure that no crashes happen with run-time option `-disablewallet`.
|
||||||
|
|
||||||
- *Rationale*: In RPC code that conditionally uses the wallet (such as
|
- *Rationale*: In RPC code that conditionally uses the wallet (such as
|
||||||
`validateaddress`) it is easy to forget that global pointer `pwalletMain`
|
`validateaddress`), it is easy to forget that global pointer `pwalletMain`
|
||||||
can be nullptr. See `test/functional/disablewallet.py` for functional tests
|
can be nullptr. See `test/functional/disablewallet.py` for functional tests
|
||||||
exercising the API with `-disablewallet`
|
exercising the API with `-disablewallet`.
|
||||||
|
|
||||||
- Include `db_cxx.h` (BerkeleyDB header) only when `ENABLE_WALLET` is set
|
- 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
|
- *Rationale*: Otherwise compilation of the disable-wallet build will fail in environments without BerkeleyDB.
|
||||||
|
|
||||||
General C++
|
General C++
|
||||||
-------------
|
-------------
|
||||||
|
@ -483,26 +485,26 @@ Guidelines](https://isocpp.github.io/CppCoreGuidelines/).
|
||||||
Common misconceptions are clarified in those sections:
|
Common misconceptions are clarified in those sections:
|
||||||
|
|
||||||
- Passing (non-)fundamental types in the [C++ Core
|
- Passing (non-)fundamental types in the [C++ Core
|
||||||
Guideline](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-conventional)
|
Guideline](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-conventional).
|
||||||
|
|
||||||
- Assertions should not have side-effects
|
- Assertions should not have side-effects.
|
||||||
|
|
||||||
- *Rationale*: Even though the source code is set to refuse to compile
|
- *Rationale*: Even though the source code is set to refuse to compile
|
||||||
with assertions disabled, having side-effects in assertions is unexpected and
|
with assertions disabled, having side-effects in assertions is unexpected and
|
||||||
makes the code harder to understand
|
makes the code harder to understand.
|
||||||
|
|
||||||
- If you use the `.h`, you must link the `.cpp`
|
- 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
|
- *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
|
not linking the other is confusing. Please avoid that. Moving functions from
|
||||||
the `.h` to the `.cpp` should not result in build errors
|
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
|
- Use the RAII (Resource Acquisition Is Initialization) paradigm where possible. For example, by using
|
||||||
`unique_ptr` for allocations in a function.
|
`unique_ptr` for allocations in a function.
|
||||||
|
|
||||||
- *Rationale*: This avoids memory and resource leaks, and ensures exception safety
|
- *Rationale*: This avoids memory and resource leaks, and ensures exception safety.
|
||||||
|
|
||||||
- Use `MakeUnique()` to construct objects owned by `unique_ptr`s
|
- Use `MakeUnique()` to construct objects owned by `unique_ptr`s.
|
||||||
|
|
||||||
- *Rationale*: `MakeUnique` is concise and ensures exception safety in complex expressions.
|
- *Rationale*: `MakeUnique` is concise and ensures exception safety in complex expressions.
|
||||||
`MakeUnique` is a temporary project local implementation of `std::make_unique` (C++14).
|
`MakeUnique` is a temporary project local implementation of `std::make_unique` (C++14).
|
||||||
|
@ -510,27 +512,27 @@ Common misconceptions are clarified in those sections:
|
||||||
C++ data structures
|
C++ data structures
|
||||||
--------------------
|
--------------------
|
||||||
|
|
||||||
- Never use the `std::map []` syntax when reading from a map, but instead use `.find()`
|
- 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
|
- *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
|
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
|
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
|
- Do not compare an iterator from one data structure with an iterator of
|
||||||
another data structure (even if of the same type)
|
another data structure (even if of the same type).
|
||||||
|
|
||||||
- *Rationale*: Behavior is undefined. In C++ parlor this means "may reformat
|
- *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
|
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,
|
- 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() +
|
including `&vch[0]` for an empty vector. Use `vch.data()` and `vch.data() +
|
||||||
vch.size()` instead.
|
vch.size()` instead.
|
||||||
|
|
||||||
- Vector bounds checking is only enabled in debug mode. Do not rely on it
|
- Vector bounds checking is only enabled in debug mode. Do not rely on it.
|
||||||
|
|
||||||
- Initialize all non-static class members where they are defined.
|
- Initialize all non-static class members where they are defined.
|
||||||
If this is skipped for a good reason (i.e., optimization on the critical
|
If this is skipped for a good reason (i.e., optimization on the critical
|
||||||
path), add an explicit comment about this
|
path), add an explicit comment about this.
|
||||||
|
|
||||||
- *Rationale*: Ensure determinism by avoiding accidental use of uninitialized
|
- *Rationale*: Ensure determinism by avoiding accidental use of uninitialized
|
||||||
values. Also, static analyzers balk about this.
|
values. Also, static analyzers balk about this.
|
||||||
|
@ -554,12 +556,12 @@ class A
|
||||||
`int8_t`. Do not use bare `char` unless it is to pass to a third-party API.
|
`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
|
This type can be signed or unsigned depending on the architecture, which can
|
||||||
lead to interoperability problems or dangerous conditions such as
|
lead to interoperability problems or dangerous conditions such as
|
||||||
out-of-bounds array accesses
|
out-of-bounds array accesses.
|
||||||
|
|
||||||
- Prefer explicit constructions over implicit ones that rely on 'magical' C++ behavior
|
- 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
|
- *Rationale*: Easier to understand what is happening, thus easier to spot mistakes, even for those
|
||||||
that are not language lawyers
|
that are not language lawyers.
|
||||||
|
|
||||||
Strings and formatting
|
Strings and formatting
|
||||||
------------------------
|
------------------------
|
||||||
|
@ -567,17 +569,17 @@ Strings and formatting
|
||||||
- Be careful of `LogPrint` versus `LogPrintf`. `LogPrint` takes a `category` argument, `LogPrintf` does not.
|
- 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
|
- *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
|
formatting mismatch, and it is easy to get wrong because of subtly similar naming.
|
||||||
|
|
||||||
- Use `std::string`, avoid C string manipulation functions
|
- Use `std::string`, avoid C string manipulation functions.
|
||||||
|
|
||||||
- *Rationale*: C++ string handling is marginally safer, less scope for
|
- *Rationale*: C++ string handling is marginally safer, less scope for
|
||||||
buffer overflows and surprises with `\0` characters. Also some C string manipulations
|
buffer overflows, and surprises with `\0` characters. Also, some C string manipulations
|
||||||
tend to act differently depending on platform, or even the user locale
|
tend to act differently depending on platform, or even the user locale.
|
||||||
|
|
||||||
- Use `ParseInt32`, `ParseInt64`, `ParseUInt32`, `ParseUInt64`, `ParseDouble` from `utilstrencodings.h` for number parsing
|
- Use `ParseInt32`, `ParseInt64`, `ParseUInt32`, `ParseUInt64`, `ParseDouble` from `utilstrencodings.h` for number parsing.
|
||||||
|
|
||||||
- *Rationale*: These functions do overflow checking, and avoid pesky locale issues.
|
- *Rationale*: These functions do overflow checking and avoid pesky locale issues.
|
||||||
|
|
||||||
- Avoid using locale dependent functions if possible. You can use the provided
|
- Avoid using locale dependent functions if possible. You can use the provided
|
||||||
[`lint-locale-dependence.sh`](/test/lint/lint-locale-dependence.sh)
|
[`lint-locale-dependence.sh`](/test/lint/lint-locale-dependence.sh)
|
||||||
|
@ -607,14 +609,14 @@ Strings and formatting
|
||||||
`wcstoll`, `wcstombs`, `wcstoul`, `wcstoull`, `wcstoumax`, `wcswidth`,
|
`wcstoll`, `wcstombs`, `wcstoul`, `wcstoull`, `wcstoumax`, `wcswidth`,
|
||||||
`wcsxfrm`, `wctob`, `wctomb`, `wctrans`, `wctype`, `wcwidth`, `wprintf`
|
`wcsxfrm`, `wctob`, `wctomb`, `wctrans`, `wctype`, `wcwidth`, `wprintf`
|
||||||
|
|
||||||
- For `strprintf`, `LogPrint`, `LogPrintf` formatting characters don't need size specifiers
|
- 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
|
- *Rationale*: Bitcoin Core uses tinyformat, which is type safe. Leave them out to avoid confusion.
|
||||||
|
|
||||||
Variable names
|
Variable names
|
||||||
--------------
|
--------------
|
||||||
|
|
||||||
Although the shadowing warning (`-Wshadow`) is not enabled by default (it prevents issues rising
|
Although the shadowing warning (`-Wshadow`) is not enabled by default (it prevents issues arising
|
||||||
from using a different variable with the same name),
|
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.
|
please name variables so that their names do not shadow variables defined in the source code.
|
||||||
|
|
||||||
|
@ -633,22 +635,20 @@ AddressBookPage::AddressBookPage(Mode _mode)
|
||||||
```
|
```
|
||||||
|
|
||||||
When using nested cycles, do not name the inner cycle variable the same as in
|
When using nested cycles, do not name the inner cycle variable the same as in
|
||||||
upper cycle etc.
|
the upper cycle, etc.
|
||||||
|
|
||||||
|
|
||||||
Threads and synchronization
|
Threads and synchronization
|
||||||
----------------------------
|
----------------------------
|
||||||
|
|
||||||
- Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential
|
- 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
|
deadlocks are introduced. As of 0.12, this is defined by default when
|
||||||
configuring with `--enable-debug`
|
configuring with `--enable-debug`.
|
||||||
|
|
||||||
- When using `LOCK`/`TRY_LOCK` be aware that the lock exists in the context of
|
- 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
|
the current scope, so surround the statement and the code that needs the lock
|
||||||
with braces
|
with braces.
|
||||||
|
|
||||||
OK:
|
OK:
|
||||||
|
|
||||||
```c++
|
```c++
|
||||||
{
|
{
|
||||||
TRY_LOCK(cs_vNodes, lockNodes);
|
TRY_LOCK(cs_vNodes, lockNodes);
|
||||||
|
@ -657,7 +657,6 @@ Threads and synchronization
|
||||||
```
|
```
|
||||||
|
|
||||||
Wrong:
|
Wrong:
|
||||||
|
|
||||||
```c++
|
```c++
|
||||||
TRY_LOCK(cs_vNodes, lockNodes);
|
TRY_LOCK(cs_vNodes, lockNodes);
|
||||||
{
|
{
|
||||||
|
@ -679,13 +678,11 @@ Scripts
|
||||||
`#!/usr/bin/env bash` searches the user's PATH to find the bash binary.
|
`#!/usr/bin/env bash` searches the user's PATH to find the bash binary.
|
||||||
|
|
||||||
OK:
|
OK:
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
#!/usr/bin/env bash
|
#!/usr/bin/env bash
|
||||||
```
|
```
|
||||||
|
|
||||||
Wrong:
|
Wrong:
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
#!/bin/bash
|
#!/bin/bash
|
||||||
```
|
```
|
||||||
|
@ -694,9 +691,9 @@ Source code organization
|
||||||
--------------------------
|
--------------------------
|
||||||
|
|
||||||
- Implementation code should go into the `.cpp` file and not the `.h`, unless necessary due to template usage or
|
- 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
|
when performance due to inlining is critical.
|
||||||
|
|
||||||
- *Rationale*: Shorter and simpler header files are easier to read, and reduce compile time
|
- *Rationale*: Shorter and simpler header files are easier to read and reduce compile time.
|
||||||
|
|
||||||
- Use only the lowercase alphanumerics (`a-z0-9`), underscore (`_`) and hyphen (`-`) in source code filenames.
|
- Use only the lowercase alphanumerics (`a-z0-9`), underscore (`_`) and hyphen (`-`) in source code filenames.
|
||||||
|
|
||||||
|
@ -714,7 +711,7 @@ Source code organization
|
||||||
- Don't import anything into the global namespace (`using namespace ...`). Use
|
- Don't import anything into the global namespace (`using namespace ...`). Use
|
||||||
fully specified types such as `std::string`.
|
fully specified types such as `std::string`.
|
||||||
|
|
||||||
- *Rationale*: Avoids symbol conflicts
|
- *Rationale*: Avoids symbol conflicts.
|
||||||
|
|
||||||
- Terminate namespaces with a comment (`// namespace mynamespace`). The comment
|
- Terminate namespaces with a comment (`// namespace mynamespace`). The comment
|
||||||
should be placed on the same line as the brace closing the namespace, e.g.
|
should be placed on the same line as the brace closing the namespace, e.g.
|
||||||
|
@ -729,7 +726,7 @@ namespace {
|
||||||
} // namespace
|
} // namespace
|
||||||
```
|
```
|
||||||
|
|
||||||
- *Rationale*: Avoids confusion about the namespace context
|
- *Rationale*: Avoids confusion about the namespace context.
|
||||||
|
|
||||||
- Use `#include <primitives/transaction.h>` bracket syntax instead of
|
- Use `#include <primitives/transaction.h>` bracket syntax instead of
|
||||||
`#include "primitives/transactions.h"` quote syntax.
|
`#include "primitives/transactions.h"` quote syntax.
|
||||||
|
@ -752,13 +749,13 @@ namespace {
|
||||||
GUI
|
GUI
|
||||||
-----
|
-----
|
||||||
|
|
||||||
- Do not display or manipulate dialogs in model code (classes `*Model`)
|
- Do not display or manipulate dialogs in model code (classes `*Model`).
|
||||||
|
|
||||||
- *Rationale*: Model classes pass through events and data from the core, they
|
- *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
|
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.
|
holds: try to not directly access core data structures from Views.
|
||||||
|
|
||||||
- Avoid adding slow or blocking code in the GUI thread. In particular do not
|
- Avoid adding slow or blocking code in the GUI thread. In particular, do not
|
||||||
add new `interfaces::Node` and `interfaces::Wallet` method calls, even if they
|
add new `interfaces::Node` and `interfaces::Wallet` method calls, even if they
|
||||||
may be fast now, in case they are changed to lock or communicate across
|
may be fast now, in case they are changed to lock or communicate across
|
||||||
processes in the future.
|
processes in the future.
|
||||||
|
@ -777,12 +774,12 @@ Subtrees
|
||||||
Several parts of the repository are subtrees of software maintained elsewhere.
|
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
|
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
|
directly upstream without being PRed directly against the project. They will be merged back in the next
|
||||||
subtree merge.
|
subtree merge.
|
||||||
|
|
||||||
Others are external projects without a tight relationship with our project. Changes to these should also
|
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
|
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.
|
quickly. Cosmetic changes should be purely taken upstream.
|
||||||
|
|
||||||
There is a tool in `test/lint/git-subtree-check.sh` to check a subtree directory for consistency with
|
There is a tool in `test/lint/git-subtree-check.sh` to check a subtree directory for consistency with
|
||||||
its upstream repository.
|
its upstream repository.
|
||||||
|
@ -812,11 +809,11 @@ you must be aware of.
|
||||||
|
|
||||||
### File Descriptor Counts
|
### File Descriptor Counts
|
||||||
|
|
||||||
In most configurations we use the default LevelDB value for `max_open_files`,
|
In most configurations, we use the default LevelDB value for `max_open_files`,
|
||||||
which is 1000 at the time of this writing. If LevelDB actually uses this many
|
which is 1000 at the time of this writing. If LevelDB actually uses this many
|
||||||
file descriptors it will cause problems with Bitcoin's `select()` loop, because
|
file descriptors, it will cause problems with Bitcoin's `select()` loop, because
|
||||||
it may cause new sockets to be created where the fd value is >= 1024. For this
|
it may cause new sockets to be created where the fd value is >= 1024. For this
|
||||||
reason, on 64-bit Unix systems we rely on an internal LevelDB optimization that
|
reason, on 64-bit Unix systems, we rely on an internal LevelDB optimization that
|
||||||
uses `mmap()` + `close()` to open table files without actually retaining
|
uses `mmap()` + `close()` to open table files without actually retaining
|
||||||
references to the table file descriptors. If you are upgrading LevelDB, you must
|
references to the table file descriptors. If you are upgrading LevelDB, you must
|
||||||
sanity check the changes to make sure that this assumption remains valid.
|
sanity check the changes to make sure that this assumption remains valid.
|
||||||
|
@ -841,14 +838,14 @@ details.
|
||||||
|
|
||||||
It is possible for LevelDB changes to inadvertently change consensus
|
It is possible for LevelDB changes to inadvertently change consensus
|
||||||
compatibility between nodes. This happened in Bitcoin 0.8 (when LevelDB was
|
compatibility between nodes. This happened in Bitcoin 0.8 (when LevelDB was
|
||||||
first introduced). When upgrading LevelDB you should review the upstream changes
|
first introduced). When upgrading LevelDB, you should review the upstream changes
|
||||||
to check for issues affecting consensus compatibility.
|
to check for issues affecting consensus compatibility.
|
||||||
|
|
||||||
For example, if LevelDB had a bug that accidentally prevented a key from being
|
For example, if LevelDB had a bug that accidentally prevented a key from being
|
||||||
returned in an edge case, and that bug was fixed upstream, the bug "fix" would
|
returned in an edge case, and that bug was fixed upstream, the bug "fix" would
|
||||||
be an incompatible consensus change. In this situation the correct behavior
|
be an incompatible consensus change. In this situation, the correct behavior
|
||||||
would be to revert the upstream fix before applying the updates to Bitcoin's
|
would be to revert the upstream fix before applying the updates to Bitcoin's
|
||||||
copy of LevelDB. In general you should be wary of any upstream changes affecting
|
copy of LevelDB. In general, you should be wary of any upstream changes affecting
|
||||||
what data is returned from LevelDB queries.
|
what data is returned from LevelDB queries.
|
||||||
|
|
||||||
Scripted diffs
|
Scripted diffs
|
||||||
|
@ -867,7 +864,7 @@ To create a scripted-diff:
|
||||||
- `-BEGIN VERIFY SCRIPT-`
|
- `-BEGIN VERIFY SCRIPT-`
|
||||||
- `-END VERIFY SCRIPT-`
|
- `-END VERIFY SCRIPT-`
|
||||||
|
|
||||||
The scripted-diff is verified by the tool `test/lint/commit-script-check.sh`. The tool's default behavior when supplied
|
The scripted-diff is verified by the tool `test/lint/commit-script-check.sh`. The tool's default behavior, when supplied
|
||||||
with a commit is to verify all scripted-diffs from the beginning of time up to said commit. Internally, the tool passes
|
with a commit is to verify all scripted-diffs from the beginning of time up to said commit. Internally, the tool passes
|
||||||
the first supplied argument to `git rev-list --reverse` to determine which commits to verify script-diffs for, ignoring
|
the first supplied argument to `git rev-list --reverse` to determine which commits to verify script-diffs for, ignoring
|
||||||
commits that don't conform to the commit message format described above.
|
commits that don't conform to the commit message format described above.
|
||||||
|
@ -900,23 +897,23 @@ RPC interface guidelines
|
||||||
|
|
||||||
A few guidelines for introducing and reviewing new RPC interfaces:
|
A few guidelines for introducing and reviewing new RPC interfaces:
|
||||||
|
|
||||||
- Method naming: use consecutive lower-case names such as `getrawtransaction` and `submitblock`
|
- Method naming: use consecutive lower-case names such as `getrawtransaction` and `submitblock`.
|
||||||
|
|
||||||
- *Rationale*: Consistency with existing interface.
|
- *Rationale*: Consistency with the existing interface.
|
||||||
|
|
||||||
- Argument naming: use snake case `fee_delta` (and not, e.g. camel case `feeDelta`)
|
- Argument naming: use snake case `fee_delta` (and not, e.g. camel case `feeDelta`)
|
||||||
|
|
||||||
- *Rationale*: Consistency with existing interface.
|
- *Rationale*: Consistency with the existing interface.
|
||||||
|
|
||||||
- Use the JSON parser for parsing, don't manually parse integers or strings from
|
- Use the JSON parser for parsing, don't manually parse integers or strings from
|
||||||
arguments unless absolutely necessary.
|
arguments unless absolutely necessary.
|
||||||
|
|
||||||
- *Rationale*: Introduces hand-rolled string manipulation code at both the caller and callee sites,
|
- *Rationale*: Introduces hand-rolled string manipulation code at both the caller and callee sites,
|
||||||
which is error prone, and it is easy to get things such as escaping wrong.
|
which is error-prone, and it is easy to get things such as escaping wrong.
|
||||||
JSON already supports nested data structures, no need to re-invent the wheel.
|
JSON already supports nested data structures, no need to re-invent the wheel.
|
||||||
|
|
||||||
- *Exception*: AmountFromValue can parse amounts as string. This was introduced because many JSON
|
- *Exception*: AmountFromValue can parse amounts as string. This was introduced because many JSON
|
||||||
parsers and formatters hard-code handling decimal numbers as floating point
|
parsers and formatters hard-code handling decimal numbers as floating-point
|
||||||
values, resulting in potential loss of precision. This is unacceptable for
|
values, resulting in potential loss of precision. This is unacceptable for
|
||||||
monetary values. **Always** use `AmountFromValue` and `ValueFromAmount` when
|
monetary values. **Always** use `AmountFromValue` and `ValueFromAmount` when
|
||||||
inputting or outputting monetary values. The only exceptions to this are
|
inputting or outputting monetary values. The only exceptions to this are
|
||||||
|
@ -925,7 +922,7 @@ A few guidelines for introducing and reviewing new RPC interfaces:
|
||||||
|
|
||||||
- Missing arguments and 'null' should be treated the same: as default values. If there is no
|
- Missing arguments and 'null' should be treated the same: as default values. If there is no
|
||||||
default value, both cases should fail in the same way. The easiest way to follow this
|
default value, both cases should fail in the same way. The easiest way to follow this
|
||||||
guideline is detect unspecified arguments with `params[x].isNull()` instead of
|
guideline is to detect unspecified arguments with `params[x].isNull()` instead of
|
||||||
`params.size() <= x`. The former returns true if the argument is either null or missing,
|
`params.size() <= x`. The former returns true if the argument is either null or missing,
|
||||||
while the latter returns true if is missing, and false if it is null.
|
while the latter returns true if is missing, and false if it is null.
|
||||||
|
|
||||||
|
@ -959,7 +956,7 @@ A few guidelines for introducing and reviewing new RPC interfaces:
|
||||||
from there.
|
from there.
|
||||||
|
|
||||||
- A RPC method must either be a wallet method or a non-wallet method. Do not
|
- A RPC method must either be a wallet method or a non-wallet method. Do not
|
||||||
introduce new methods that differ in behavior based on presence of a wallet.
|
introduce new methods that differ in behavior based on the presence of a wallet.
|
||||||
|
|
||||||
- *Rationale*: as well as complicating the implementation and interfering
|
- *Rationale*: as well as complicating the implementation and interfering
|
||||||
with the introduction of multi-wallet, wallet and non-wallet code should be
|
with the introduction of multi-wallet, wallet and non-wallet code should be
|
||||||
|
@ -967,7 +964,7 @@ A few guidelines for introducing and reviewing new RPC interfaces:
|
||||||
|
|
||||||
- Try to make the RPC response a JSON object.
|
- Try to make the RPC response a JSON object.
|
||||||
|
|
||||||
- *Rationale*: If a RPC response is not a JSON object then it is harder to avoid API breakage if
|
- *Rationale*: If a RPC response is not a JSON object, then it is harder to avoid API breakage if
|
||||||
new data in the response is needed.
|
new data in the response is needed.
|
||||||
|
|
||||||
- Wallet RPCs call BlockUntilSyncedToCurrentChain to maintain consistency with
|
- Wallet RPCs call BlockUntilSyncedToCurrentChain to maintain consistency with
|
||||||
|
|
Loading…
Reference in a new issue