docs: add CONTRIBUTING.md (#2452)
This commit is contained in:
Charles Kerr
GitHub
parent
6c582469fa
commit
e0c3a7f3e6
|
|
@ -0,0 +1,56 @@
|
|||
# Contributing to Transmission
|
||||
|
||||
Thanks for reading this and thinking about contributing! :tada:
|
||||
|
||||
This page is a list of suggestions and guidelines for contributing. They're not rules, just guidelines. Use your best judgment and feel free to propose changes to this document in a pull request.
|
||||
|
||||
# If you've got a change in mind
|
||||
|
||||
New people usually start volunteering because they have an itch they want to scratch. If you already know what you want to work on first, please comment in an existing issue, or [file a new issue](https://github.com/transmission/transmission/issues/new/choose) or [start a new discussion](https://github.com/transmission/transmission/discussions/new)! The maintainers will try to get you the information you need.
|
||||
|
||||
# If you're looking for ideas
|
||||
|
||||
If not, there are three labels in the issues tracker that can help:
|
||||
|
||||
- [`help wanted`](https://github.com/transmission/transmission/issues?q=is%3Aissue+is%3Aopen+label%3A%22help+wanted%22) indicates that the issue is stuck and needs an outside developer. This is usually because some domain expertese is needed, e.g. for a specific platform or external API.
|
||||
- [`good first issue`](https://github.com/transmission/transmission/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22) is a `pr-welcome` issue that is probably on the easier side to code.
|
||||
- [`pr welcome`](https://github.com/transmission/transmission/issues?q=is%3Aissue+is%3Aopen+label%3A%22pr+welcome%22) is for features that have been requested and which that the project doesn't have resources to implement, but would consider adding if a good PR was submitted.
|
||||
|
||||
The project also welcomes changes that:
|
||||
|
||||
- improve Transmission's compliance with [accepted BEPs](https://www.bittorrent.org/beps/bep_0000.html)
|
||||
- improve transfer speeds or peer communication
|
||||
- reduce the app's footprint in CPU or memory use
|
||||
- improve testing
|
||||
- simplify / shrink the existing codebase
|
||||
- remove deprecated macOS API use in the macOS client
|
||||
- remove deprecated GTK API use in the GTK client
|
||||
- reduce feature disparity between the different Transmission apps
|
||||
|
||||
# Mechanics
|
||||
|
||||
## Getting Started
|
||||
|
||||
On macOS, Transmission is usually built with Xcode. Everywhere else, it's CMake + the development environment of your choice. If you need to add source files but don't have Xcode, a maintainer can help you to update the Xcode project file. See [README.md](README.md) for information on building Transmission from source.
|
||||
|
||||
## Style
|
||||
|
||||
- Try to follow the [C++ core guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines).
|
||||
- Prefer memory-managed objects over raw pointers
|
||||
- Prefer `constexpr` over `#define`
|
||||
- Prefer `enum class` over `enum`
|
||||
- Prefer new-style headers, e.g. `<cstring>` over `<string.h>`
|
||||
- Fix any warnings in new code before merging
|
||||
- Run `./code-style.sh` on your code to ensure the whole codebase has consistent indentation.
|
||||
|
||||
Note that Transmission existed in C for over a decade and those idioms don't change overnight. "Follow the C++ core guidelines" can be difficult when working with older code, and the maintainers will understand that when reviewing your PRs. :smiley:
|
||||
|
||||
## Considerations
|
||||
|
||||
- Prefer commonly-used tools over bespoke ones, e.g. use `std::list` instead of rolling your own list. This simplifies the code and makes it easier for other contributors to work with.
|
||||
- Please keep new code reasonably decoupled from the rest of the codebase for testability, either with DI or other methods. Be aware that much of the codebase was not written with testability in mind. See peer-mgr-wishlist for one example of adding new, tested code into an existing untested module.
|
||||
- When adding advanced features, consider exposing them only in the config file instead of the UI.
|
||||
- Transmission has a native macOS app, a native GTK app, and a Qt app. This is a strength in that each client can tightly integrate with its target environment, but it comes at the cost of making GUI changes very time-consuming. So consider, does this feature _need_ to be in the GUI?
|
||||
- New features must be reachable via the C API and the RPC/JSON API.
|
||||
- The macOS and GTK clients still use the C API. Everything else, including a large number of 3rd party applications, use the RPC/JSON API. New features need to be usable via both of these.
|
||||
- KISS. Transmission is a _huge_ codebase so if you're trying to decide between to approaches to implement something, try the simpler one first.
|
||||
|
|
@ -1,95 +0,0 @@
|
|||
# Notes on the C-to-C++ Conversion
|
||||
|
||||
- libtransmission was written in C for fifteen years, so eliminating all Cisms is nearly impossible. **Modernization
|
||||
patches are welcomed** but it won't all happen overnight. `tr_strdup()` and `constexpr` wil exist side-by-side in the
|
||||
codebase for the forseeable future.
|
||||
|
||||
- It's so tempting to refactor all the things! Please keep modernization patches reasonably focused so that they will be
|
||||
easy to review.
|
||||
|
||||
- Prefer `std::` tools over bespoke ones. For example, use `std::vector`
|
||||
instead of tr_ptrArray. Redundant bespoke code should be removed.
|
||||
|
||||
- Consider ripple effects before adding C++ into public headers. Will it break C code that #includes that header? If you think it might, [consult with downstream projects](https://github.com/transmission/transmission/issues/1826) to see if this is a problem for them.
|
||||
|
||||
## Checklist for modernization of a module
|
||||
|
||||
> **_NOTE:_**
|
||||
> The version in `libtransmission/CMakeLists.txt` is C++17.
|
||||
> See https://github.com/AnthonyCalandra/modern-cpp-features
|
||||
|
||||
This can be done in multiple smaller passes:
|
||||
|
||||
1. Satisfy clang-tidy. `libtransmission/.clang-tidy`'s list of rules should eventually be expanded to a list similar to the one in `qt/.clang-tidy`.
|
||||
2. Group member functions and their parent structs
|
||||
- Use `namespace libtransmission`
|
||||
- Split large modules into smaller groups of files in a `libtransmission/<name>` subdirectories, with own
|
||||
sub-namespace.
|
||||
3. Enums replaced with new `enum class` syntax. Numeric `#define` constants replaced with C++ `const`/`constexpr`.
|
||||
4. Memory - promote init and free functions to C++ ctors and dtors, and ensure it is only managed with new/delete
|
||||
5. Owned memory - promote simple pointer fields owning their data to smart pointers (unique_ptr, shared_ptr, vector,
|
||||
string)
|
||||
|
||||
### Detailed Steps
|
||||
|
||||
1. Satisfy clang-tidy warnings
|
||||
- Change C includes to C++ wraps, example: <string.h> becomes <cstring> and update calls to standard library to
|
||||
use `std::` namespace prefix. This clearly delineates the border between std library and transmission. Headers
|
||||
must be sorted alphabetically.
|
||||
- Headers which are used conditionally based on some `#ifdef` in the code, should also have same `#ifdef` in the
|
||||
include section.
|
||||
- Revisit type warnings, `int` vs `unsigned int`. Sizes and counts should use `size_t` type where this does not
|
||||
break external API declarations. Ideally change that too.
|
||||
2. Move and group code together.
|
||||
- Move member functions into structs. To minimize code churn, create function forward declarations inside structs,
|
||||
and give `struct_name::` prefixes to the functions.
|
||||
```c++
|
||||
typedef struct {
|
||||
int field;
|
||||
} foo;
|
||||
int foo_blep(struct foo *f) {
|
||||
return f->field;
|
||||
}
|
||||
```
|
||||
becomes:
|
||||
```c++
|
||||
struct foo {
|
||||
int field;
|
||||
void blep();
|
||||
};
|
||||
int foo::blep() {
|
||||
return this->field;
|
||||
}
|
||||
```
|
||||
- For functions taking `const` pointer, add `const` after the function prototype: `int blep() const` like so.
|
||||
- For structs used by other modules, struct definitions should relocate to internal `*-common.h` header files.
|
||||
- Split large files into sub-modules sharing own separate sub-namespace and sitting in a subdirectory
|
||||
under `libtransmission/`.
|
||||
- Some externally invoked functions must either not move OR have `extern "C"` adapter functions.
|
||||
|
||||
3. Enums promoted to `enum class` and given a type:
|
||||
```c++
|
||||
enum { A, B, C };
|
||||
```
|
||||
becomes
|
||||
```c++
|
||||
enum: int { A, B, C }; // unscoped, use A, B, C
|
||||
enum MyEnum: int { A, B, C }; // unscoped, use A, B, C
|
||||
// OR wrap into a scope -
|
||||
enum struct MyEnum: int { A, B, C }; // scoped, use MyEnum::A
|
||||
enum class MyEnum: int { A, B, C }; // scoped, use MyEnum::A
|
||||
```
|
||||
this will make all values of enum to have that numeric type.
|
||||
|
||||
4. Numeric/bool `#define` constants should be replaced with C++ `const`/`constexpr`.
|
||||
|
||||
6. Memory management:
|
||||
- Prefer constructors and destructors vs manual construction and destruction. But when doing so must ensure that the
|
||||
struct is never constructed using C `malloc`/`free`, but must use C++ `new`/`delete`.
|
||||
- Avoid using std::memset on a new struct. It is allowed in C, and C++ struct but will destroy virtual table on a
|
||||
C++ class. Use field initializers in C++.
|
||||
|
||||
7. Owned memory:
|
||||
- If destructor deletes something, it means it was owned. Promote that field to owning type (vector, unique_ptr,
|
||||
shared_ptr or string).
|
||||
|
||||
Loading…
Reference in New Issue