doc: Add non-style-related development guidelines
I've collected these over time, mostly adding notes after troubleshooting obscure bugs. As I hope to get the community more involved in the whole process, I think it is useful to add to the developer-notes.
This commit is contained in:
Wladimir J. van der Laan
parent
3ac7060934
commit
b8c06ef409
|
|
@ -204,3 +204,172 @@ 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 pulls 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 that no crashes happen with run-time option `-disablewallet`.
|
||||
|
||||
- *Rationale*: In RPC code that conditionally use 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 are the interface for the implementation file. 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
|
||||
`scoped_pointer` 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 vector out-of-bounds exceptions. `&vch[0]` is illegal for an
|
||||
empty vector, `&vch[vch.size()]` is always illegal. Use `begin_ptr(vch)` and
|
||||
`end_ptr(vch)` to get the begin and end pointer instead (defined in
|
||||
`serialize.h`)
|
||||
|
||||
- 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
|
||||
------------------------
|
||||
|
||||
- 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, 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
|
||||
|
||||
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.
|
||||
|
|
|
|||
Loading…
Reference in a new issue