1# Contributing 2 3Feedback and contributions are very welcome! 4 5Here's help on how to make contributions, divided into the following sections: 6 7The quick-read part: 8* general information, 9* vulnerability reporting, 10* documentation. 11 12The long-read part: 13* code changes, 14* how to check proposed changes before submitting them, 15* reuse of other libraries, frameworks etc. 16 17## General information 18 19For specific proposals, please provide them as 20[pull requests](https://github.com/quotient-im/libQuotient/pulls) 21or 22[issues](https://github.com/quotient-im/libQuotient/issues) 23For general discussion, feel free to use our Matrix room: 24[#quotient:matrix.org](https://matrix.to/#/#quotient:matrix.org). 25 26If you're new to the project (or FLOSS in general), 27[this is a list of good first issues](https://github.com/quotient-im/libQuotient/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22) 28that don't require much knowledge about the project. 29You are welcome aboard! 30 31### Pull requests and different branches recommended 32 33Pull requests are preferred, since they are specific. 34See the GitHub Help [articles about pull requests](https://help.github.com/articles/using-pull-requests/) 35to learn how to deal with them. 36 37We recommend creating different branches for different (logical) 38changes, and creating a pull request when you're done into the master branch. 39See the GitHub documentation on 40[creating branches](https://help.github.com/articles/creating-and-deleting-branches-within-your-repository/) 41and 42[using pull requests](https://help.github.com/articles/using-pull-requests/). 43 44### How we handle proposals 45 46We use GitHub to track all changes via its 47[issue tracker](https://github.com/quotient-im/libQuotient/issues) and 48[pull requests](https://github.com/quotient-im/libQuotient/pulls). 49Specific changes are proposed using those mechanisms. 50Issues are assigned to an individual who works on it and then marks it complete. 51If there are questions or objections, the conversation area of that 52issue or pull request is used to resolve it. 53 54<!-- 55### Developer Certificate of Origin (DCO) - not enforced yet 56 57All contributions (including pull requests) must agree to 58the [Developer Certificate of Origin (DCO) version 1.1](doc/dco.txt). 59This is exactly the same one created and used by the Linux kernel developers 60and posted on <http://developercertificate.org/>. 61This is a developer's certification that he or she has the right to 62submit the patch for inclusion into the project. 63 64Simply submitting a contribution implies this agreement, however, 65please include a "Signed-off-by" tag in every patch 66(this tag is a conventional way to confirm that you agree to the DCO). 67You can do this with <tt>git commit --signoff</tt> (the <tt>-s</tt> flag 68is a synonym for <tt>--signoff</tt>). 69 70Another way to do this is to write the following at the end of the commit 71message, on a line by itself separated by a blank line from the body of 72the commit: 73 74 Signed-off-by: YOUR NAME <YOUR.EMAIL@EXAMPLE.COM> 75 76You can signoff by default in this project by creating a file 77(say "git-template") that contains 78some blank lines and the signed-off-by text above; 79then configure git to use that as a commit template. For example: 80 81 git config commit.template ~/cii-best-practices-badge/git-template 82 83It's not practical to fix old contributions in git, so if one is forgotten, 84do not try to fix them. We presume that if someone sometimes used a DCO, 85a commit without a DCO is an accident and the DCO still applies. 86--> 87### License 88 89Unless a contributor explicitly specifies otherwise, we assume contributors 90to agree that all contributed code is released either under *LGPL v2.1 or later*. 91The project plans to switch to LGPL v3 for library code in the near future. 92<!-- The below is invalid yet! 93All new contributed material that is not executable, including all text when not executed, is also released under the 94[Creative Commons Attribution 4.0 International (CC BY 4.0) license](https://creativecommons.org/licenses/by/4.0/) or later. 95--> 96 97Any components proposed for reuse should have a license that permits releasing 98a derivative work under *LGPL v2.1 or later* or LGPL v3. Moreover, the license of 99a proposed component should be approved by OSI, no exceptions. 100 101## Vulnerability reporting (security issues) - see [SECURITY.md](./SECURITY.md) 102 103## Documentation changes 104 105Most of the documentation is in Markdown format. All Markdown files use the .md 106filename extension. Any help on fixing/extending these is more than welcome. 107 108Where reasonable, limit yourself to Markdown that will be accepted by different 109markdown processors (e.g., what is specified by CommonMark or the original 110Markdown). In practice, as long as libQuotient is hosted at GitHub, 111[GFM (GitHub-flavoured Markdown)](https://help.github.com/articles/github-flavored-markdown/) 112is used to show those files in a browser, so it's fine to use its extensions. 113In particular, you can mark code snippets with the programming language used; 114blank lines separate paragraphs, newlines inside a paragraph do *not* force a line break. 115 116Beware - this is *not* the same markdown algorithm used by GitHub when it 117renders issue or pull comments; in those cases 118[newlines in paragraph-like content are considered as real line breaks](https://help.github.com/articles/writing-on-github/); 119unfortunately this other algorithm is *also* called GitHub-flavoured markdown. 120(Yes, it'd be better if there were different names for different things.) 121 122In your markdown, please don't use tab characters and avoid "bare" URLs. 123In a hyperlink, the link text and URL should be on the same line. 124While historically we didn't care about the line length in markdown texts 125(and more often than not put the whole paragraph into one line), this is subject 126to change anytime soon, with 80-character limit _recommendation_ 127(which is softer than the limit for C/C++ code) imposed on everything 128_except hyperlinks_ (because wrapping hyperlinks breaks the rendering). 129 130Do not use trailing two spaces for line breaks, since these cannot be seen 131and may be silently removed by some tools. If, for whatever reason, a blank line 132is not an option, use <tt><br /></tt> (an HTML break). 133 134## End of TL;DR 135 136If you don't plan/have substantial contributions, you can stop reading here. 137Further sections are for those who's going to actively hack on the library code. 138 139## Code changes 140 141The code should strive to be DRY (don't repeat yourself), clear, and obviously 142correct (i.e. buildable). Some technical debt is inevitable, 143just don't bankrupt us with it. Refactoring is welcome. 144 145### Code style and formatting 146 147As of Quotient 0.6, the C++ standard for newly written code is C++17, with a few 148restrictions, notably: 149* standard library's _deduction guides_ cannot be used to lighten up syntax 150 in template instantiation, i.e. you have to still write 151 `std::array<int, 2> { 1, 2 }` instead of `std::array { 1, 2 }` or use helpers 152 like `std::make_pair` - once we move over to the later Apple toolchain, this 153 will be no more necessary. 154* enumerators and slots cannot have `[[attributes]]` because moc of Qt 5.9 155 chokes on them. This will be lifted when we move on to Qt 5.12 for the oldest 156 supported version. 157* things from `std::filesystem` cannot be used until we push the oldest 158 required g++/libc to version 8. 159 160The code style is defined by `.clang-format`, and in general, all C++ files 161should follow it. Files with minor deviations from the defined style are still 162accepted in PRs; however, unless explicitly marked with `// clang-format off` 163and `// clang-format on`, these deviations will be rectified any commit soon 164after. 165 166Additional considerations: 167* 4-space indents, no tabs, no trailing spaces, no last empty lines. If you 168 spot the code abusing these - thank you for fixing it. 169* Prefer keeping lines within 80 characters. Slight overflows are ok only 170 if that helps readability. 171* Please don't make "hypocritical structs" with protected or private members. 172 In general, `struct` is used to denote a plain-old-data structure, rather 173 than data+behaviour. If you need access control or are adding yet another 174 non-trivial (construction, assignment) member function to a `struct`, 175 just make it a `class` instead. 176* For newly created classes, keep to 177 [the rule of 3/5/0](http://en.cppreference.com/w/cpp/language/rule_of_three) - 178 make sure to read about the rule of zero if you haven't before, it's not 179 what you might think it is. 180* Qt containers are generally preferred to STL containers; however, there are 181 notable exceptions, and libQuotient already uses them: 182 * `std::array` and `std::deque` have no direct counterparts in Qt. 183 * Because of COW semantics, Qt containers cannot hold uncopyable classes. 184 Classes without a default constructor are a problem too. Examples of that 185 are `SyncRoomData` and `EventsArray<>`. Use STL containers for those but 186 see the next point and also consider if you can supply a reasonable 187 copy/default constructor. 188 * STL containers can be freely used in code internal to a translation unit 189 (i.e., in a certain .cpp file) _as long as that is not exposed in the API_. 190 It's ok to use, e.g., `std::vector` instead of `QVector` to tighten up 191 code where you don't need COW, or when dealing with uncopyable 192 data structures (see the previous point). However, exposing STL containers 193 in the API is not encouraged (except where absolutely necessary, e.g. we use 194 `std::deque` for a timeline). Exposing STL containers or iterators in API 195 intended for usage by QML code (e.g. in `Q_PROPERTY`) is unlikely to work 196 and therefore unlikely to be accepted into `master`. 197 * Prefer using `std::unique_ptr<>` over `QScopedPointer<>` as it gives 198 stronger guarantees. Earlier revisions of this text recommended using 199 `QScopedPointer<>` because Qt Creator's debugger UI had a display helper 200 for it; it now has helpers for both. 201* Use `QVector` instead of `QList` where possible - see the 202 [great article by Marc Mutz on Qt containers](https://marcmutz.wordpress.com/effective-qt/containers/) 203 for details. 204 205### API conventions 206 207Calls, data structures and other symbols not intended for use by clients 208should _not_ be exposed in (public) .h files, unless they are necessary 209to declare other public symbols. In particular, this involves private members 210(functions, typedefs, or variables) in public classes; use pimpl idiom to hide 211implementation details as much as possible. `_impl` namespace is reserved for 212definitions that should not be used by clients and are not covered by 213API guarantees. 214 215Note: As of now, all header files of libQuotient are considered public; 216this may change eventually. 217 218### Comments 219 220Whenever you add a new call to the library API that you expect to be used 221from client code, you must supply a proper doc-comment along with the call. 222Doxygen style is preferred; but Javadoc is acceptable too. Some parts are 223not documented at all; adding doc-comments to them is highly encouraged. 224 225Use `\brief` for the summary, and follow with details after 226an empty doc-comment line. 227 228For in-code comments, the advice is as follows: 229* Don't restate what's happening in the code unless it's not really obvious. 230 We assume the readers to have at least some command of C++ and Qt. If your 231 code is not obvious, consider making it clearer itself before commenting. 232* Both C++ and Qt still come with their arcane features and dark corners, 233 and we don't want to limit anybody who feels they have a case for 234 variable templates, raw literals, or use `std::as_const` to avoid container 235 detachment. Use your experience to figure what might be less well-known to 236 readers and comment such cases (references to web pages, Quotient wiki etc. 237 are very much ok, the previous bullet notwithstanding). 238* Make sure to document not so much "what" but more "why" certain code is done 239 the way it is. In the worst case, the logic of the code can be 240 reverse-engineered; but you can almost never reverse-engineer the line of 241 reasoning and the pitfalls avoided. 242 243### Automated tests 244 245There's no testing framework as of now; either Catch or Qt Test or both will 246be used eventually. 247 248The `tests/` directory contains a command-line program, quotest, used for 249automated functional testing. Any significant addition to the library API 250should be accompanied by a respective test in quotest. To add a test you should: 251- Add a new test to the `TestSuite` class (technically, each test is a private 252 slot and there are two macros, `TEST_DECL()` and `TEST_IMPL()`, that conceal 253 passing the testing handle in `thisTest` variable to the test method). 254- Add test logic to the slot, using `FINISH_TEST` macro to assert the test 255 outcome and complete the test (`FINISH_TEST` contains `return`). ALL 256 (even failing) branches should conclude with a `FINISH_TEST` (or `FAIL_TEST` 257 that is a shortcut for a failing `FINISH_TEST`) invocation, unless you 258 intend to have a "DID NOT FINISH" message in the logs in certain conditions. 259 260The `TestManager` class sets up some basic test fixture to help you with testing; 261notably, the tests can rely on having an initialised `Room` object for the test 262room in `targetRoom` member variable. PRs to introduce a proper testing framework 263are very welcome (make sure to migrate tests from quotest though). Note that 264tests can go async, which is the biggest hurdle for Qt Test adoption. 265 266### Security, privacy, and performance 267 268Pay attention to security, and work *with*, not against, the usual security hardening practices (however few in C++). 269 270`char *` and similar unchecked C-style read/write arrays are forbidden - use 271Qt containers or at the very least `std::array<>` instead. Where you see fit 272(usually with data structures), try to use smart pointers, especially 273`std::unique_ptr<>` or `QScopedPointer` instead of bare pointers. When dealing 274with `QObject`s, use the parent-child ownership semantics exercised by Qt 275(this is preferred to using smart pointers). If you find a particular use case 276where the strict semantic of unique pointers doesn't help and a shared pointer 277is necessary, feel free to step up with the working code and it will be 278considered for inclusion. 279 280Exercise the [principle of least privilege](https://en.wikipedia.org/wiki/Principle_of_least_privilege) where reasonable and appropriate. Prefer less-coupled cohesive code. 281 282Protect private information, in particular passwords and email addresses. 283Absolutely _don't_ spill around this information in logs, and only display 284those in UI where really needed. Do not forget about local access to data 285(in particular, be very careful when storing something in temporary files, 286let alone permanent configuration or state). Avoid mechanisms that could be 287used for tracking where possible (we do need to verify people are logged in 288but that's pretty much it), and ensure that third parties can't use interactions 289for tracking. Matrix protocols evolve towards decoupling 290the personally identifiable information from user activity entirely - follow 291this trend. 292 293We want the software to have decent performance for users even on weaker 294machines. At the same time we keep libQuotient single-threaded as much 295as possible, to keep the code simple. That means being cautious about operation 296complexity (read about big-O notation if you need a kickstart on the topic). 297This especially refers to operations on the whole timeline and the list of 298users - each of these can have tens of thousands of elements so even operations 299with linear complexity, if heavy enough (with I/O or complex processing), 300can produce noticeable GUI freezing or stuttering. When you don't see a way 301to reduce algorithmic complexity, embed occasional `processEvents()` invocations 302in heavy loops (see `Connection::saveState()` to get the idea). 303 304Having said that, there's always a trade-off between various attributes; 305in particular, readability and maintainability of the code is more important 306than squeezing every bit out of that clumsy algorithm. Beware of premature 307optimization and profile the code before before diving into hardcore tweaking 308that might not give the benefits you think it would. 309 310Speaking of profiling logs (see README.md on how to turn them on) - if you 311expect some code to take considerable (more than 10k "simple operations") time 312you might want to setup a `QElapsedTimer` and drop the elapsed time into logs 313under `PROFILER` logging category (see the existing code for examples - 314`room.cpp` has quite a few). In order to reduce small timespan logging spam, 315`PROFILER` log lines are usually guarded by a check that the timer counted 316some considerable time (200 microseconds by default, 20 microseconds for 317tighter parts). It's possible to override this limit library-wide by passing 318the new value (in microseconds) in `PROFILER_LOG_USECS` definition to 319the compiler; I don't think anybody ever used this facility. If you used it, 320and are reading this text - let me (`@kitsune:matrix.org`) know. 321 322### Generated C++ code for CS API 323The code in `lib/csapi`, `lib/identity` and `lib/application-service`, although 324it resides in Git, is actually generated from the official Swagger/OpenAPI 325definition files. If you're unhappy with something in there and want to improve 326the code, you have to understand the way these files are produced and setup 327some additional tooling. The shortest possible procedure resembling 328the below text can be found in .github/workflows/ci.yml (our CI configuration 329actually regenerates those files upon every build). As described below, there 330is also a handy build target for CMake. 331 332#### Why generate the code at all? 333Because otherwise we have to do monkey business of writing boilerplate code, 334with the same patterns, types etc., literally, for every single API endpoint, 335and one of libQuotient authors got fed up with it at some point in time. 336By then about 15 job classes were written; the entire API is about 100 endpoints 337and counting. Besides, the existing jobs had to be updated according to changes 338in CS API that have been, and will keep, coming. Other considerations can be 339found in [this talk about API description languages](https://youtu.be/W5TmRozH-rg) 340that also briefly touches on GTAD. 341 342#### Prerequisites for CS API code generation 3431. Get the source code of GTAD and its dependencies, e.g. using the command: 344 `git clone --recursive https://github.com/KitsuneRal/gtad.git` 3452. Build GTAD: in the source code directory, do `cmake . && cmake --build .` 346 (you might need to pass `-DCMAKE_PREFIX_PATH=<path to Qt>`, 347 similar to libQuotient itself). 3483. Get the Matrix CS API definitions that are included in a matrix-doc repo. 349 You can `git clone https://github.com/matrix-org/matrix-doc.git`, 350 the official repo; it's recommended though to instead 351 `git clone https://github.com/quotient-im/matrix-doc.git` - this repo closely 352 follows the official one, with an additional guarantee that you can always 353 generate working code for the main libQuotient branch from its HEAD commit. 354 And of course you can use your own repository if you need to change the API 355 definition. 3564. If you plan to submit a PR or just would like the generated code to be 357 properly formatted, you should either ensure you have clang-format 358 (version 9 at least) in your PATH or pass the _absolute_ path to it by adding 359 `-DCLANG_FORMAT=<absolute path>` to the CMake invocation below. 360 361#### Generating CS API contents 3621. Pass additional configuration to CMake when configuring libQuotient: 363 `-DMATRIX_DOC_PATH=<path to matrix-doc repo> -DGTAD_PATH=<path to gtad binary (not the repo!)>`. 364 If everything's right, these two CMake variables will be mentioned in 365 CMake output and will trigger configuration of an additional build target, 366 see the next step. 3672. Generate the code: `cmake --build <your build dir> --target update-api`; 368 if you use CMake with GNU Make, you can just do `make update-api` instead. 369 Building this target will create (overwriting without warning) `.h` and `.cpp` 370 files in `lib/csapi`, `lib/identity`, `lib/application-service` for all 371 YAML files it can find in `matrix-doc/api/client-server` and other files 372 in `matrix-doc/api` these depend on. 3733. Re-run CMake so that the build system knows about new files, if there are any 374 (this step is unnecessary if you use CMake 3.12 or later). 375 376#### Changing generated code 377See the more detailed description of what GTAD is and how it works in the documentation on GTAD in its source repo. Only parts specific for libQuotient are described here. 378 379GTAD uses the following three kinds of sources: 3801. OpenAPI files. Each file is treated as a separate source (if you worked with 381 swagger-codegen - you do _not_ need to have a single file for the whole API). 3822. A configuration file, in our case it's `gtad/gtad.yaml` - this one is common 383 for all OpenAPI files GTAD is invoked on. 3843. Source code template files: `gtad/*.mustache` - are also common. 385 386The Mustache files have a templated (not in C++ sense) definition of a network 387job, deriving from BaseJob; if necessary, data structure definitions used 388by this job are put before the job class. Bigger Mustache files look a bit 389hideous for a newcomer; and the only known highlighter that can handle 390the combination of Mustache (originally a web templating language) and C++ is 391provided in CLion IDE. Fortunately, all our Mustache files are reasonably 392concise and well-formatted these days. 393To simplify things some reusable Mustache blocks are defined in `gtad.yaml` - 394see its `mustache:` section. Adventurous souls that would like to figure 395what's going on in these files should speak up in the Quotient room - 396I (Kitsune) will be very glad to help you out. 397 398The types map in `gtad.yaml` is the central switchboard when it comes to matching OpenAPI types with C++ (and Qt) ones. It uses the following type attributes aside from pretty obvious "imports:": 399* `avoidCopy` - this attribute defines whether a const ref should be used instead of a value. For basic types like int this is obviously unnecessary; but compound types like `QVector` should rather be taken by reference when possible. 400* `moveOnly` - some types are not copyable at all and must be moved instead (an obvious example is anything "tainted" with a member of type `std::unique_ptr<>`). The template will use `T&&` instead of `T` or `const T&` to pass such types around. 401* `useOmittable` - wrap types that have no value with "null" semantics (i.e. number types and custom-defined data structures) into a special `Omittable<>` template defined in `converters.h` - a substitute for `std::optional` from C++17 (we're still at C++14 yet). 402* `omittedValue` - an alternative for `useOmittable`, just provide a value used for an omitted parameter. This is used for bool parameters which normally are considered false if omitted (or they have an explicit default value, passed in the "official" GTAD's `defaultValue` variable). 403* `initializer` - this is a _partial_ (see GTAD and Mustache documentation for explanations but basically it's a variable that is a Mustache template itself) that specifies how exactly a default value should be passed to the parameter. E.g., the default value for a `QString` parameter is enclosed into `QStringLiteral`. 404 405Instead of relying on the event structure definition in the OpenAPI files, `gtad.yaml` uses pointers to libQuotient's event structures: `EventPtr`, `RoomEventPtr` and `StateEventPtr`. Respectively, arrays of events, when encountered in OpenAPI definitions, are converted to `Events`, `RoomEvents` and `StateEvents` containers. When there's no way to figure the type from the definition, an opaque `QJsonObject` is used, leaving the conversion to the library and/or client code. 406 407## How to check proposed changes before submitting them 408 409Checking the code on at least one configuration is essential; if you only have 410a hasty fix that doesn't even compile, better make an issue and put a link to 411your commit into it (with an explanation what it is about and why). 412 413### Standard checks 414 415The following warnings configuration is applied with GCC and Clang when using CMake: 416`-W -Wall -Wextra -pedantic -Werror=return-type -Wno-unused-parameter -Wno-gnu-zero-variadic-macro-arguments` 417(the last one is to mute a warning triggered by Qt code for debug logging). 418We don't turn most of the warnings to errors but please treat them as such. 419If you use Qt Creator, the following line can be used with the Clang code model: 420`-Weverything -Werror=return-type -Wno-c++98-compat -Wno-c++98-compat-pedantic -Wno-unused-macros -Wno-newline-eof -Wno-exit-time-destructors -Wno-global-constructors -Wno-gnu-zero-variadic-macro-arguments -Wno-documentation -Wno-missing-prototypes -Wno-shadow-field-in-constructor -Wno-padded -Wno-weak-vtables -Wno-unknown-attributes -Wno-comma -Wno-string-conversion -Wno-return-std-move-in-c++11`. 421 422### Continuous Integration 423 424We use CI to check buildability and smoke-testing on Linux (GCC, Clang), 425MacOS (Clang), and Windows (MSVC). Every PR will go through these, and you'll 426see the traffic lights from them on the PR page. If your PR fails 427on any platform double-check that it's not your code causing it - and fix 428(or ask how to fix if you don't know) if it is. 429 430### clang-format 431 432We strongly recommend using clang-format or, even better, use an IDE that 433supports it. This will lay over a tedious task of following the assumed 434code style from your shoulders (and fingers) to your computer. 435 436### Other tools 437 438Recent versions of Qt Creator and CLion can automatically run your code through 439clang-tidy. The following list of clang-tidy checks gives a good insight 440without too many false positives: 441`-*,bugprone-argument-comment,bugprone-assert-side-effect,bugprone-bool-pointer-implicit-conversion,bugprone-copy-constructor-init,bugprone-dangling-handle,bugprone-fold-init-type,bugprone-forward-declaration-namespace,bugprone-forwarding-reference-overload,bugprone-inaccurate-erase,bugprone-integer-division,bugprone-lambda-function-name,bugprone-macro-*,bugprone-move-forwarding-reference,bugprone-multiple-statement-macro,bugprone-parent-virtual-call,bugprone-signed-char-misuse,bugprone-sizeof-*,bugprone-string-constructor,bugprone-string-integer-assignment,bugprone-suspicious-*,bugprone-terminating-continue,bugprone-undefined-memory-manipulation,bugprone-undelegated-constructor,bugprone-unused-*,bugprone-use-after-move,bugprone-virtual-near-miss,cert-dcl03-c,cert-dcl21-cpp,cert-dcl50-cpp,cert-dcl54-cpp,cert-dcl58-cpp,cert-env33-c,cert-err09-cpp,cert-err34-c,cert-err52-cpp,cert-err60-cpp,cert-err61-cpp,cert-fio38-c,cert-flp30-c,cert-msc30-c,cert-msc50-cpp,cert-oop11-cpp,clang-analyzer-apiModeling.StdCLibraryFunctions,clang-analyzer-core.CallAndMessage,clang-analyzer-core.NullDereference,clang-analyzer-cplusplus.*,clang-analyzer-optin.cplusplus.*,cppcoreguidelines-c-copy-assignment-signature,cppcoreguidelines-non-private-member-variables-in-classes,cppcoreguidelines-pro-type-cstyle-cast,cppcoreguidelines-slicing,hicpp-deprecated-headers,hicpp-invalid-access-moved,hicpp-member-init,hicpp-move-const-arg,hicpp-new-delete-operators,hicpp-static-assert,hicpp-undelegated-constructor,hicpp-use-*,misc-*,-misc-definitions-in-headers,-misc-no-recursion,-misc-non-private-member-variables-in-classes,modernize-loop-convert,modernize-pass-by-value,modernize-return-braced-init-list,modernize-shrink-to-fit,modernize-unary-static-assert,modernize-use-*,-modernize-use-trailing-return-type,performance-*,-performance-no-automatic-move,-performance-noexcept-move-constructor,-performance-unnecessary-*,readability-*,-readability-braces-around-statements,-readability-implicit-bool-conversion,-readability-isolate-declaration,-readability-magic-numbers,-readability-named-parameter,-readability-qualified-auto`. 442 443Qt Creator, in addition, knows about clazy, an even deeper Qt-aware static 444analysis tool that produces some notices about Qt-specific issues that are 445easy to overlook otherwise, such as possible unintended copying of 446a Qt container, or unguarded null pointers. You can use this time to time 447(see Analyze menu in Qt Creator) instead of hogging your machine with 448deep analysis as you type (or after each saving, depending on your version 449of Qt Creator). Most of clazy checks are relevant to our code, except: 450`fully-qualified-moc-types,overloaded-signal,qstring-comparison-to-implicit-char,foreach,non-pod-global-static,qstring-allocations,jni-signatures,qt4-qstring-from-array`. 451 452### Submitting API changes 453 454If you changed the API definitions, the path to upstream becomes somewhat 455intricate, as you have to coordinate with two projects, making up to 4 PRs along 456the way. The recommended sequence depends on whether or not you have to 457[write a Matrix Spec Change aka MSC](https://matrix.org/docs/spec/proposals). 458Usually you have to, unless your API changes keep API semantics intact. 459In that case: 4601. Submit an MSC before submitting changes to the API definition files and 461 libQuotient. 4622. The MSC gets reviewed by the Spec Core Team. This can be a lengthy process 463 but it's necessary for the Matrix ecosystem integrity. 4643. When your MSC has at least some approvals (not necessarily a complete 465 acceptance but at least some approvals should be there) submit a PR to 466 libQuotient, referring to your `matrix-doc` repo. Make sure that generated 467 files are committed separately from non-generated ones (no need to make two 468 PRs; just separate them in different commits). 4694. If your libQuotient PR is approved and MSC is not there yet you'll be asked 470 to submit a PR with API definition files at 471 `https://github.com/quotient-im/matrix-doc`. Note that this is _not_ 472 an official repo; but you can refer to your libQuotient PR as 473 an _implementation_ of the MSC - a necessary step before making a so-called 474 "spec PR". 4755. Once MSC is accepted, submit your `matrix-doc` changes as a PR to 476 `https://github.com/matrix-org/matrix-doc` (the "spec PR" mentioned above). 477 This will require that your submission meets the standards set by this 478 project (they are quite reasonable and not too hard to meet). 479 480If your changes don't need an MSC, it becomes a more straightforward combination 481of 2 PRs: one to `https://github.com/matrix-org/matrix-doc` ("spec PR") and one 482to libQuotient (with the same guidance about putting generated and non-generated 483files in different commits). 484 485## Git commit messages 486 487When writing git commit messages, try to follow the guidelines in 488[How to Write a Git Commit Message](https://chris.beams.io/posts/git-commit/): 489 4901. Separate subject from body with a blank line 4912. Be reasonable on the subject line length, because this is what we see in commit logs. Try to fit in 50 characters whenever possible. 4923. Capitalize the subject line 4934. Do not end the subject line with a period 4945. Use the imperative mood in the subject line (*command* form) 495 (we don't always practice this ourselves but let's try). 4966. Use the body to explain what and why vs. how 497 (git tracks how it was changed in detail, don't repeat that). Sometimes a quick overview of "how" is acceptable if a commit is huge - but maybe split a commit into smaller ones, to begin with? 498 499## Reuse (libraries, frameworks, etc.) 500 501C++ is unfortunately not very coherent about SDK/package management, and we try to keep building the library as easy as possible. Because of that we are very conservative about adding dependencies to libQuotient. That relates to additional Qt components and even more to other libraries. Fortunately, even the Qt components now in use (Qt Core and Network) are very feature-rich and provide plenty of ready-made stuff. 502 503Regardless of the above paragraph (and as mentioned earlier in the text), we're now looking at possible options for futures and automated testing, so PRs onboarding those will be considered with much gratitude. 504 505Some cases need additional explanation: 506* Before rolling out your own super-optimised container or algorithm written 507 from scratch, take a good long look through documentation on Qt and 508 C++ standard library. Please try to reuse the existing facilities 509 as much as possible. 510* You should have a good reason (or better several ones) to add a component 511 from KDE Frameworks. We don't rule this out and there's no prejudice against 512 KDE; it just so happened that KDE Frameworks is one of most obvious 513 reuse candidates but so far none of these components survived 514 as libQuotient deps. So we are cautious. Extra notice to KDE folks: 515 I'll be happy if an addon library on top of libQuotient is made using 516 KDE facilities, and I'm willing to take part in its evolution; but please 517 also respect LXDE people who normally don't have KDE frameworks installed. 518* Never forget that libQuotient is aimed to be a non-visual library; 519 QtGui in dependencies is only driven by (entirely offscreen) dealing with 520 QImages. While there's a bunch of visual code (in C++ and QML) shared 521 between Quotient-enabled _applications_, this is likely to end up 522 in a separate (Quotient-enabled) library, rather than libQuotient itself. 523 524## Attribution 525 526This text is based on CONTRIBUTING.md from CII Best Practices Badge project, which is a collective work of its contributors (many thanks!). The text itself is licensed under CC-BY-4.0. 527