From 3dd2fc0093d78bd975c98c798b441b8fe267ff6a Mon Sep 17 00:00:00 2001 From: whitequark Date: Sat, 10 Dec 2016 06:09:35 +0000 Subject: [PATCH] Add CONTRIBUTING.md. --- CONTRIBUTING.md | 158 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 158 insertions(+) create mode 100644 CONTRIBUTING.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000..9dc6855 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,158 @@ +Contributing to SolveSpace +========================== + +Contributing bug reports +------------------------ + +Bug reports are always welcome! When reporting a bug, please include the following: + + * The version of SolveSpace (use Help → About...); + * The operating system; + * The save file that reproduces the incorrect behavior, or, if trivial or impossible, + instructions for reproducing it. + +GitHub does not allow attaching `*.slvs` files, but it does allow attaching `*.zip` files, +so any savefiles should first be archived. + +Contributing code +----------------- + +SolveSpace is written in C++, and currently targets all compilers compliant with C++11. +This includes GCC 5 and later, Clang 3.3 and later, and Visual Studio 12 (2013) and later. + +### High-level conventions + +#### Portability + +SolveSpace aims to consist of two general parts: a fully portable core, and platform-specific +UI and support code. Anything outside of `src/platform/` should only use standard C++11, +and rely on `src/platform/unixutil.cpp` and `src/platform/w32util.cpp` to interact with +the OS where this cannot be done through the C++11 standard library. + +#### Libraries + +SolveSpace primarily relies on the C++11 STL. STL has well-known drawbacks, but is also +widely supported, used, and understood. SolveSpace also includes a fair amount of use of +bespoke containers List and IdList; these provide STL iterators, and can be used when +convenient, such as when reusing other code. + +One notable departure here is the STL I/O threads. SolveSpace does not use STL I/O threads +for two reasons: (i) the interface is borderline unusable, and (ii) on Windows it is not +possible to open files with Unicode paths through STL. + +When using external libraries (other than to access platform features), the libraries +should satisfy the following conditions: + + * Portable, and preferably not interacting with the platform at all; + * Can be included as a CMake subproject, to facilitate Windows, Android, etc. builds; + * Use a license less restrictive than GPL (BSD/MIT, Apache2, MPL, etc.) + +#### String encoding + +Internally, SolveSpace exclusively stores and uses UTF-8 for all purposes; any `std::string` +may be assumed to be encoded in UTF-8. On Windows, UTF-8 strings are converted to and from +wide strings at the boundary; see [UTF-8 Everywhere][utf8] for details. + +[utf8]: http://utf8everywhere.org/ + +#### String formatting + +For string formatting, a wrapper around `sprintf`, `ssprintf`, is used. A notable +pitfall when using it is trying to pass an `std::string` argument without first converting +it to a C string with `.c_str()`. + +#### Filesystem access + +For filesystem access, the C standard library is used. The `ssfopen` and `ssremove` +wrappers are provided that accept UTF-8 encoded paths. + +#### Assertions + +To ensure that internal invariants hold, the `ssassert` function is used, e.g. +`ssassert(!isFoo, "Unexpected foo condition");`. Unlike the standard `assert` function, +the `ssassert` function is always enabled, even in release builds. It is more valuable +to discover a bug through a crash than to silently generate incorrect results, and crashes +do not result in losing more than a few minutes of work thanks to the autosave feature. + +### Use of C++ features + +The conventions described in this section should be used for all new code, but there is a lot +of existing code in SolveSpace that does not use them. This is fine; don't touch it if it works, +but if you need to modify it anyway, might as well modernize it. + +#### Exceptions + +Exceptions are not used primarily because SolveSpace's testsuite uses measurement +of branch coverage, important for the critical parts such as the geometric kernel. +Every function call with exceptions enabled introduces a branch, making branch coverage +measurement useless. + +#### Operator overloading + +Operator overloading is not used primarily for historical reasons. Instead, method such +as `Plus` are used. + +#### Member visibility + +Member visibility is not used for implementation hiding. Every member field and function +is `public`. + +#### Constructors + +Constructors are not used for initialization, chiefly because indicating an error +in a constructor would require throwing an exception, nor does it use constructors for +blanket zero-initialization because of the performance impact of doing this for common +POD classes like `Vector`. + +Instances can be zero-initialized using the aggregate-initialization syntax, e.g. `Foo foo = {};`. +This zero-initializes the POD members and default-initializes the non-POD members, generally +being an equivalent of `memset(&foo, 0, sizeof(foo));` but compatible with STL containers. + +#### Input- and output-arguments + +Functions accepting an input argument take it either by-value (`Vector v`) or +by-const-reference (`const Vector &v`). Generally, passing by-value is safer as the value +cannot be aliased by something else, but passing by-const-reference is faster, as a copy is +eliminated. Small values should always be passed by-value, and otherwise functions that do not +capture pointers into their arguments should take them by-const-reference. Use your judgement. + +Functions accepting an output argument always take it by-pointer (`Vector *v`). This makes +it immediately visible at the call site as it is seen that the address is taken. Arguments +are never passed by-reference, except when needed for interoperability with STL, etc. + +#### Iteration + +`foreach`-style iteration is preferred for both STL and `List`/`IdList` containers as it indicates +intent clearly, as opposed to `for`-style. + +#### Const correctness + +Functions that do not mutate `this` should be marked as `const`; when iterating a collection +without mutating any of its elements, `for(const Foo &elem : collection)` is preferred to indicate +the intent. + +### Coding style + +Code is formatted by the following rules: + + * Code is indented using 4 spaces, with no trailing spaces, and lines are wrapped + at 100 columns; + * Braces are placed at the end of the line with the declaration or control flow statement; + * Braces are used with every control flow statement, even if there is only one statement + in the body; + * There is no space after control flow keywords (`if`, `while`, etc.); + * Identifiers are formatted in camel case; variables start with a lowercase letter + (`exampleVariable`) and functions start with an uppercase letter (`ExampleFunction`). + +For example: + +```c++ +std::string SolveSpace::Dirname(std::string filename) { + int slash = filename.rfind(PATH_SEP); + if(slash >= 0) { + return filename.substr(0, slash); + } + + return ""; +} +```