This shows you the differences between two versions of the page.
Both sides previous revisionPrevious revisionNext revision | Previous revision | ||
contributing_guide [2016-07-13 07:01] – /* Private implementations */ skyjake | devel:developer_guidelines [2017-03-20 12:34] (current) – ↷ Page name changed from devel:contributing_guide to devel:developer_guidelines skyjake | ||
---|---|---|---|
Line 1: | Line 1: | ||
+ | ====== Developer guidelines ====== | ||
+ | |||
+ | |||
+ | Doomsday Engine is a diverse project that is free and open-source. | ||
+ | As such, one of many ways to give back to Doomsday is by contributing code. | ||
+ | |||
+ | Be it a bugfix, new feature, or even just cleanup, your work can help make Doomsday the most | ||
+ | technically advanced Doom engine available today. | ||
+ | |||
+ | When working with multiple persons on a project, especially one with a large amount of code, | ||
+ | such as Doomsday or the Linux kernel, it is imperative that there exist guidelines for that code | ||
+ | which is submitted by collaborators such that the project can be approached and understood by all involved | ||
+ | and third parties. | ||
+ | |||
+ | This document is meant to be a summary of both requirements and best practices for submitting code. It covers, | ||
+ | to the extent deemed necessary various guidelines for submitting code, such as: | ||
+ | |||
+ | * Code formatting | ||
+ | * Language features | ||
+ | * Naming conventions | ||
+ | * Version control practices | ||
+ | |||
+ | If you, as a prospective contributor, | ||
+ | you are encouraged to contact a project manager for answers, which may then be added to this document. | ||
+ | |||
+ | |||
+ | ===== Code conventions ===== | ||
+ | |||
+ | |||
+ | Doomsday is composed of various components, the majority of which are written in both C and C++. | ||
+ | Doomsday aims to support the C99 and C++11 (C++0x) language sets respectively, | ||
+ | |||
+ | |||
+ | ===== C++ ===== | ||
+ | |||
+ | |||
+ | When writing or modifying C++ for Doomsday, the following guidelines must be adhered to. | ||
+ | |||
+ | |||
+ | ==== Indentation ==== | ||
+ | |||
+ | |||
+ | Indentation should reflect scope such that code residing in a scope //n// times more restricted should be | ||
+ | indented //n// levels. Spaces must be used instead of tab stops, and one indentation level must be 4 spaces wide. | ||
+ | |||
+ | The following code demonstrates indentation in accordance with the scope: | ||
+ | |||
+ | <code c++> | ||
+ | class Something : public Thing | ||
+ | { | ||
+ | public: | ||
+ | // Implementation | ||
+ | | ||
+ | Something(string param) | ||
+ | : Thing(param) | ||
+ | { | ||
+ | // Do things | ||
+ | } | ||
+ | |||
+ | private: | ||
+ | // Implementation details | ||
+ | }; | ||
+ | | ||
+ | static void doSomething(bool predicate) | ||
+ | { | ||
+ | // Nested type | ||
+ | // This is ideally at the top of the method. Use your judgement. | ||
+ | struct NestedStructure | ||
+ | { | ||
+ | // Members | ||
+ | }; | ||
+ | | ||
+ | { // Nested scope block | ||
+ | doSomethingOther(); | ||
+ | } | ||
+ | | ||
+ | if (predicate) | ||
+ | { | ||
+ | doSomething(); | ||
+ | } | ||
+ | } | ||
+ | | ||
+ | { // Scope block | ||
+ | int scopedInt = 2; | ||
+ | doSomethingMore(scopedInt); | ||
+ | } // New line | ||
+ | </ | ||
+ | |||
+ | As far as switches go, it must be indented in a readable manner. How this is done is up to the writer. | ||
+ | The following is an example of a well-indented switch. | ||
+ | |||
+ | <code c++> | ||
+ | switch (predicate) | ||
+ | { | ||
+ | case CASE_1: | ||
+ | doSomething(); | ||
+ | break; | ||
+ | |||
+ | case CASE_SCOPED: | ||
+ | doSomething(); | ||
+ | } | ||
+ | break; // Break resides outside the scope block. | ||
+ | |||
+ | case CASE_FALL_THROUGH_CODE: | ||
+ | doSomething(); | ||
+ | case CASE_FALL_THROUGH: | ||
+ | default: | ||
+ | break; | ||
+ | } | ||
+ | </ | ||
+ | |||
+ | |||
+ | ==== Brace style ==== | ||
+ | |||
+ | |||
+ | Doomsday, for the most part, follows [[http:// | ||
+ | formatting. | ||
+ | |||
+ | Braces should not reside at a level of indentation less than that of the declaration to which they belong. | ||
+ | They must always begin on the line // | ||
+ | |||
+ | Once more, in regards to switches, should you need a scoped case, you may either place the beginning of the | ||
+ | scope block immediately proceeding the case ('' | ||
+ | |||
+ | |||
+ | ==== Flow control ==== | ||
+ | |||
+ | |||
+ | You should use a space to separate the predicate following an '' | ||
+ | predicates, the predicate may be broken at an appropriate place, and the rest put on the following line. | ||
+ | Be sure that multiline predicates are aligned sensibly — use your intuition. | ||
+ | |||
+ | |||
+ | <code c++> | ||
+ | if (option && option2 && (compound && option) | ||
+ | || nextLineOption) | ||
+ | { | ||
+ | // do something | ||
+ | } | ||
+ | </ | ||
+ | |||
+ | |||
+ | === Return points === | ||
+ | |||
+ | |||
+ | In regards to other flow control keywords, such as '' | ||
+ | you should avoid nesting in order to create more readable code. If you have excessive return points in a void method, | ||
+ | in the interest of readability you should look at reorganizing to use conditionals, | ||
+ | |||
+ | |||
+ | <code c++> | ||
+ | void tooManyReturns() | ||
+ | { | ||
+ | // some code up here | ||
+ | | ||
+ | if (!thing1) return; | ||
+ | if (!thing2) return; | ||
+ | if (!thing3 == VALUE) return; | ||
+ | | ||
+ | // more code here | ||
+ | } | ||
+ | | ||
+ | // This can be rewritten as | ||
+ | | ||
+ | void conditionalExecution() | ||
+ | { | ||
+ | // some code up here | ||
+ | | ||
+ | if (thing1 && thin2 && (thing3 == VALUE)) | ||
+ | { | ||
+ | // more code here | ||
+ | } | ||
+ | } | ||
+ | </ | ||
+ | |||
+ | |||
+ | === Predicates === | ||
+ | |||
+ | |||
+ | Regarding conditional flow control, such as '' | ||
+ | |||
+ | |||
+ | <code c++> | ||
+ | int that = 10; | ||
+ | | ||
+ | if (that) | ||
+ | { | ||
+ | // code | ||
+ | } | ||
+ | | ||
+ | // Better rewritten as | ||
+ | | ||
+ | if (that != 0) | ||
+ | { | ||
+ | // code | ||
+ | } | ||
+ | | ||
+ | // Or, in some cases `that` should always be positive | ||
+ | | ||
+ | if (that > 0) | ||
+ | { | ||
+ | // code | ||
+ | } | ||
+ | </ | ||
+ | |||
+ | This allows a reader to infer that it is neither a boolean value or a pointer, but rather a number or something which | ||
+ | overrides one of the pictured operators. | ||
+ | |||
+ | Regarding assigment or field declaration in predicates, this practice should be avoided wherever possible. | ||
+ | If the need for this is apparent, then it should be strictly for a field declared in the predicate. For instance, | ||
+ | the following code demonstrates proper assignment in a predicate: | ||
+ | |||
+ | <code c++> | ||
+ | if (Thing *thingPointer = getThingPointer()) | ||
+ | { | ||
+ | // do things with thingPointer | ||
+ | } | ||
+ | </ | ||
+ | |||
+ | Whereas the following code demonstrates cases where this behavior is either unecessary, or inappropriate. | ||
+ | |||
+ | <code c++> | ||
+ | // Innapropriate use: | ||
+ | | ||
+ | Thing *thingPointer = getThingPointer(); | ||
+ | // do stuff with thingPointer | ||
+ | if (thingPointer = getAnotherThingPointer()) | ||
+ | { | ||
+ | // Don't do this! | ||
+ | // Erroneous field reuse should be avoided at all costs | ||
+ | } | ||
+ | | ||
+ | // do more with thingPointer | ||
+ | // Don't do the above either, | ||
+ | // If a field is needed outside the scope of the | ||
+ | // conditional expressions branch(es), then it should | ||
+ | // not be modified within the expressions predicate | ||
+ | | ||
+ | // Unecessary use | ||
+ | Thing *thingPointer = getThingPointer(); | ||
+ | if (Foo *fooPointer = doBar()) | ||
+ | { | ||
+ | // use thing pointer in here | ||
+ | // use foo pointer in here | ||
+ | } | ||
+ | | ||
+ | // don't use thingPointer past this point | ||
+ | </ | ||
+ | |||
+ | In the former of the above two situations, placing both declarations in a scope block | ||
+ | along with the if statement would be preferrable. | ||
+ | |||
+ | In summary, the predicate should never modify an existing field, and should only declare one if it | ||
+ | is syntactically clear and leads to more idiomatic code. Otherwise, alternative approaches should be | ||
+ | preferred, especially if they do not pollute the parent scope. | ||
+ | |||
+ | |||
+ | === goto === | ||
+ | |||
+ | |||
+ | '' | ||
+ | behavior. If you encounter a method using '' | ||
+ | The same applies for other such similar operators. | ||
+ | |||
+ | < | ||
+ | |||
+ | |||
+ | ==== Preprocessor ==== | ||
+ | |||
+ | |||
+ | As far as preprocessor formatting goes, it is preferable that the //pound// (''#'' | ||
+ | however, directives may reside at an indentation level with respect to surrounding code. | ||
+ | |||
+ | <code c++> | ||
+ | #if condition | ||
+ | # if condtion2 | ||
+ | # | ||
+ | # else | ||
+ | # | ||
+ | # endif | ||
+ | #endif | ||
+ | </ | ||
+ | |||
+ | It is advisible that the preprocessor not be used to define constants when they could be defined natively using '' | ||
+ | This is advantageous as native constants respect scope, and may furthermore be debugged more easily. | ||
+ | |||
+ | |||
+ | ==== Private implementations ==== | ||
+ | |||
+ | |||
+ | We recommend that C++ classes use the PIMPL pattern as it encourages correctly separating the public and internal parts of a class, and makes it possible to retain ABI compatibility even when the implementation of the class changes. You can find macros in **de/ | ||
+ | |||
+ | Example of a class declaration in a **.h** file: | ||
+ | |||
+ | |||
+ | <code c++> | ||
+ | class MyClass | ||
+ | { | ||
+ | public: | ||
+ | MyClass(); | ||
+ | |||
+ | private: | ||
+ | DENG2_PRIVATE(d) | ||
+ | }; | ||
+ | </ | ||
+ | |||
+ | The implementation in a **.cpp** file: | ||
+ | |||
+ | <code c++> | ||
+ | DENG2_PIMPL(MyClass) | ||
+ | { | ||
+ | // private members | ||
+ | int a = 0; | ||
+ | |||
+ | Impl(Public *i) : Base(i) | ||
+ | { | ||
+ | // `Public` is a typedef for `MyClass`, `Base:: | ||
+ | } | ||
+ | |||
+ | ~Impl() | ||
+ | { | ||
+ | // destruct | ||
+ | } | ||
+ | }; | ||
+ | |||
+ | MyClass:: | ||
+ | {} | ||
+ | |||
+ | // ~MyClass not necessary because `d` will be automatically deleted. | ||
+ | </ | ||
+ | |||
+ | |||
+ | ==== Member order ==== | ||
+ | |||
+ | |||
+ | In class definitions, | ||
+ | Type declarations and constants isolated to a single source unit may be placed either at the beginning, or within the narrowest scope possible. | ||
+ | |||
+ | |||
+ | ==== Naming conventions ==== | ||
+ | |||
+ | |||
+ | Classes, Enumerations, | ||
+ | and method parameters should be named using '' | ||
+ | * '' | ||
+ | * '' | ||
+ | |||
+ | An underscore prefix is used for private members: | ||
+ | * '' | ||
+ | * '' | ||
+ | |||
+ | Constants, regardless of scope, and macros, should be named using '' | ||
+ | |||
+ | |||
+ | ==== Const ==== | ||
+ | |||
+ | |||
+ | '' | ||
+ | |||
+ | The '' | ||
+ | |||
+ | |||
+ | | ||
+ | | ||
+ | |||
+ | |||
+ | |||
+ | ===== C ===== | ||
+ | |||
+ | |||
+ | C code should be formatted similarly to C++, with some exceptions. | ||
+ | |||
+ | |||
+ | ==== Naming conventions ==== | ||
+ | |||
+ | |||
+ | Enumerations, | ||
+ | |||
+ | |||
+ | ===== Doxygen ===== | ||
+ | |||
+ | |||
+ | Comments in Doxygen format describing the purpose, usage, parameters, return values, errors, and other information about functions is encouraged everywhere. Place Doxygen documentation about methods/ | ||
+ | |||
+ | For example: | ||
+ | |||
+ | <code c++> | ||
+ | /** | ||
+ | * Does magical things to foobars. | ||
+ | * | ||
+ | * @param fooBar | ||
+ | * | ||
+ | * @return | ||
+ | | ||
+ | */ | ||
+ | bool Foobar_DoMagic(foobar_t *fooBar); | ||
+ | </ | ||
+ | |||
+ | |||
+ | ===== Version control ===== | ||
+ | |||
+ | |||
+ | Doomsday uses Git and [[https:// | ||
+ | known as commits, and pushed it to a fork of the official Doomsday Engine repository. Once you are ready to have your | ||
+ | work reintegrated, | ||
+ | |||
+ | This section contains guidelines to which you should adhere in order to insure that your work can be integrated | ||
+ | without issue. | ||
+ | |||
+ | |||
+ | ==== Commit messages ==== | ||
+ | |||
+ | |||
+ | When writing a commit message, you should keep in mind that the intended audience for the message is not only other developers but also the end users who read build reports to see what has changed in each build. Therefore, the message should contain enough detail to be understood on its own. If nothing else, the commit message should explain motivations behind the changes (if they are not obvious, like a bugfix, cleanup, or referencing a tracker issue). | ||
+ | |||
+ | Your commit message should contain //tags// that describe the type of commit (e.g. '' | ||
+ | * At-a-glance summary of what the commit is about. | ||
+ | * Group commits in the build reports generated by the [[devel: | ||
+ | * Index the commits in the [[:codex]]. | ||
+ | |||
+ | While the tags are not checked for validity, you should generally use one of the commonly used tags as listed in [[http:// | ||
+ | |||
+ | Doomsday Engine uses an external [[http:// | ||
+ | in your commit message, where '' | ||
+ | commit to the relevant issue. | ||
+ | |||
+ | An example commit might look like this: | ||
+ | |||
+ | < | ||
+ | |||
+ | Turns out the foobar was accessing a null pointer. | ||
+ | |||
+ | IssueID #666 | ||
+ | </ | ||
+ | |||
+ | ==== Commit etiquette ==== | ||
+ | |||
+ | |||
+ | Avoid keeping work-in-progress commits in your commit history. If you need to save your work before checking out a commit or | ||
+ | branch, you may use '' | ||
+ | |||
+ | Commits should generally represent the completion of a goal. As such, it is // | ||
+ | but not critical, though it should be noted that whether or not a desired commit may be built successfully can and | ||
+ | will effect the change of getting your pull request merged. | ||
+ | |||
+ | |||
+ | ==== Feature branches ==== | ||
+ | |||
+ | |||
+ | When working on multiple tasks at once, you can work on feature branches should you wish to avoid cross-contaminating | ||
+ | code. A feature branch is a branch from the working master that contains a set of changes specific to one task or | ||
+ | feature. An added advantage of feature branches is the ability to open a pull request specifically with that | ||
+ | feature branch' | ||
+ | |||
+ | |||
+ | ==== Integrate often ==== | ||
+ | |||
+ | |||
+ | In order to insure the smoothest merge, among other things, try to pull from your base often (e.g. '' | ||
+ | If you must make a merge commit (without conflicts), not that it will be ignored by the tracker. If you have to resolve conflicts, | ||
+ | it is your decision whether they are trivial enough (i.e. some newlines in a comment) to be lumped in to a commit to the likes of '' | ||
+ | or if a more descriptive commit is needed. | ||
+ | |||
+ | Failure to integrate regularly means that, should you choose to integrate changes from upstream in to your working | ||
+ | base, you can run in to unpleasant merge conflicts; alternatively, | ||
+ | they might run in to similar merge conflicts. | ||
+ | |||
+ | |||
+ | ===== Documentation ===== | ||
+ | |||
+ | |||
+ | Always write appropriate API documentation for your class APIs using Doxygen. | ||
+ | |||
+ | While/after implementing a feature, document it here in the wiki: | ||
+ | |||
+ | * User/ | ||
+ | * Modding/map author features should be categorized into . | ||
+ | * Technical/ | ||
+ | |||
+ | Certain documentation is processed with [[devel: | ||