Difference between revisions of "Coding Style (OpenApoc)"
m |
|||
Line 78: | Line 78: | ||
if (a == 0) | if (a == 0) | ||
</pre> | </pre> | ||
− | * References and pointers: & and | + | * References and pointers: & and * align to right (to variable) not type |
<pre> | <pre> | ||
− | float | + | float *pointerToFloat; |
</pre> | </pre> | ||
Line 484: | Line 484: | ||
Comments: | Comments: | ||
− | * either // or / | + | * either // or /* */ is fine - prefer // for single line |
* If doing multi line /\*-style comments have an aligned '\*' with at the beginning of each subseqent line: | * If doing multi line /\*-style comments have an aligned '\*' with at the beginning of each subseqent line: | ||
<pre> | <pre> | ||
/* first line | /* first line | ||
− | + | * second line | |
− | + | * last line */ | |
</pre> | </pre> | ||
* Don't comment for the sake of it | * Don't comment for the sake of it |
Revision as of 19:57, 9 March 2019
This document specifies the guidelines for writing and formatting the c++ code that forms the core of OpenApoc.
Globally, we use 'standard' c++11. This requires reasonably modern compilers (gcc 4.7+, MSVC 2013+ , clang 3.6+ have been tested). You should avoid using compiler-specific stuff where possible. Exceptions to this exist, but should be wrapped in a #ifdef:
#ifdef _MSC_VER MSVCIsCrazySometimes #else // Everything else we support (gcc + clang) are pretty much extension-compatible GCCIsntMuchBetter #endif
C++11 features are heavily encouraged - patterns from 'older' c++ versions that have been superceded should be avoided.
The formatting sections of this document are enforced by the clang-format tool. Currently the '4.0' version of clang-format
is to be used. The configuration file .clang-format
in the root of the OpenApoc source repository should match the formatting guidelines specified below.
With this, it is highly recommended to run clang-format
on all modified files before check-in. This can be run on source files with the following command:
clang-format path/to/file.cpp path/to/file.h
When ran from the root of the OpenApoc source repository it should automatically use the supplied .clang-format
configuration file. The tool also supports modifying the supplied source files to match the configured format when passed the -i
flag:
clang-format -i path/to/file.cpp path/to/file.h
When using the CMake
build system (as used on Unix-based platforms), there is a format-sources
make target that will run clang-format -i
on all source files within the OpenApoc repositry tree. This provides a single command that can be run before commiting:
make format-sources
In the future, it may be possible to use clang-tidy
, also from the clang project, to enforce more of the non-formatting sections of this style guideline. One example of this is an identifier name checking tool.
Indent
- Tabs for indenting, spaces for alignment, indenting by 1 tab for each new scope
void function() { reallyLongFunctionNameWithLotsOfArguments(argOne, argTwo, argThree); }
- Avoid going over 100 cols (at tab width of '4' spaces).
- If you find yourself going over this it's often a hint to try to pull things out of loops/into functions
- Don't break strings up to fit this, it looks ugly and makes things even harder to read.
- If you have to break, indent the following line by an extra tab
- When breaking a single statement, break the line before the next operator. Avoid having an operator as the last thing on a line.
void reallyLongFunctionNameIMeanThisIsReallyBadlyNamedWhateverIDontCareTheyPayMeAnyway(int parameterOne, int paramTwo, char theThirdOne) { if (parameterOne == yetAnotherReallyLongLineHereComeSomeWordsBlaBlaBlaAreYouStillReadingThisComeOnDont && youHaveBetterThingsToDo) { doWhatever(); } }
Whitespace
- Spaces before and after operators
a = b; a && b; a + b;
- Space after if/when/else/for, space after :/; in for
for (auto &a: b)
- No spaces after function name (or function-like keywords like 'sizeof'), but space after flow control keywords, space after comma for multiple args
func(a, b); if (a == 0)
- References and pointers: & and * align to right (to variable) not type
float *pointerToFloat;
Scope
- Indent 1 tab for each new scope
- New scope is _always_ surrounded by {} braces
- New scope has a '{' on the /next/ line at the indent of the old scope(not the new scope)
- closing scope '}' same indent at opening '{', again on a new line
- New scope caused by:
- Functions
void functionDefinition() { newScopeHere(); }
- new conditional block (if/else/when/for)
if (x) { doWhatever(); } else if (y) { doWhateverTheSecond(); } else { doThatLastThing(); }
- 'switch'
- 'case' also indents a new scope. {} are optional, based on if new stack variables are needed to handle the case.
- Note switch should always have a default case unless over an enum class (where they may not if (and only if) every value is handled)
- All 'case' sections should have a 'break'
switch (a) { case A: doSomething(); break; case B: { auto var = somethingElse(); doSomethingElse(var); break; } default: doDefaultCase(); break; }
- Class/enum/struct declarations
- note: public/private/protected are an exception to this, being aligned to 'class' keyword, not 'within' it's scope
- Class/enum/struct declarations
class MyClass { private: int privateVariable; public: void publicFunction(); };
- Exception to this is 'trivial' functions that have the definition & contents all on line line
- 'Trivial' is defined by a single statement that fits within the 100-column limit
int Class::function() {return 0;}
- New scope is not caused by:
- namespace (which should also have a comment stating the namespace name at the closing bracket)
namespace OpenApoc { Class MyClass { private: int x; }; }; // namespace OpenApoc
- labels
- Labels and #preprocessor directives /always/ on column 0 (start of line) no matter the scope
#if defined(LINUX) x = linuxFunction(); #else x = otherFunction(); #endif if (x) { goto error; } error: return 0;
Case
- Namespaces should be CamelCase
namespace OpenApoc
- Classes and enums should be CamelCase
class MyClass
- 'enum class' members should be CamelCase
enum class MyEnum { ValueOne, ValueTwo, };
- class methods and member variables (public/private/protected) should be camelBack
class MyClass { public: void someFunction(); int someVariable; };
- Function parameters (public/private/protected) should be camelBack
int function(int parameterOne, char secondOne)
- Variables should be camelBack
- Don't be afraid to use 'short' variable names if it's obvious
void function() { int localVariable = 0; int x = 1; int y = 5; for (int i = 0; i < 5; i++)
- class/global constants should be SHOUTY_CAPS, along with _all_ macros
#define OPENAPOC_VERSION "1.0"
- Labels should be lower_case:
exit_loop: goto exit_loop;
- All members should be initialised in all constructors if they don't have a default constructor. You can use member initialisation in the class definition if this is clearer
class MyClass { int variableMember = 0; };
Types
- Avoid typedef - use 'struct' keyword where necessary in c-like code
struct MyStruct { int x; }; void myStructUser(struct MyStruct s)
- up<> sp<> wp<> aliases are defined for std::unique_ptr<>, std::shared_ptr<>, std::weak_ptr<> in "library/sp.h". Use them instead of the verbose versions.
- Use anonymous namespaces for 'file-local' stuff (instead of static, as you can wrap classes in it too)
namespace { void localFunction() }; // anonymous namespace
- We provide a "UString" class. This should be used for _all_ strings, as it provides platform-local non-ascii charset handling
- All "char*"/std::string params are assumed to be in utf8 format.
Templates
- If templates help, go ahead, don't avoid them
- prefer 'typename' to 'class'
- template types should be CamelCase
template <typename LocalType> function(LocalType param)
- 'short' typenames are OK if it's obvious what's going on
template <typename T> Class<T>::function()
Class declarations
- member functions camelCase()
- 'public:' 'private:' 'protected:' are indented to the 'class' keyword, everything within them indented to class scope.
- Always use 'private', even if that's the default
class MyClass { private: int localVariable; public: void publicFunction(); };
- 'virtual' keyword only used for base class, 'override' used for derived
- All classes with a virtual (or overrided) function _must_ specify a virtual destructor
- Inheritence should be on the same line as the 'class' keyword (until you get to the column limit and break)
class BaseClass { public: virtual ~BaseClass(); virtual void someFunction(); }; class SubClass : public BaseClass { public: void someFunction() override; };
- Never use both 'virtual' and 'override
- Don't define an empty {} body in the header for constructors/destructors etc. - use '= default' instead
class MyBaseClass { public: virtual ~MyBaseClass() = default; };
- define pure virtual "virtual void func() = 0;" for 'interface' classes
class MyInterface { public: virtual void functionBaseClassesMustOverride() = 0; };
- No need for 'pure' interface classes, they can have code that all subclasses will use!
- For trivial initial values prefer initialisers in the class declaration (It's easier to see what's set and cleans up constructor definitions)
class MyClass { public: Type initialisedMember = 0; };
- In constructors prefer initialisation of members with initialiser list over assignment
- Good:
MyClass::MyClass(Type value) : member(value) { doWhatever(); }
- Bad:
MyClass::MyClass(Type value) { member = value; doWhatever(); }
- Initialisers should be in order of declaration in the class
- For example with the class:
class MyClass { public: Type memberA; Type memberB; MyClass(Type valueA, Type valueB); };
- Good:
MyClass::MyClass(Type valueA, Type valueB) : memberA(valueA), memberB(valueB) {}
- Bad:
MyClass::MyClass(Type valueA, Type valueB) : memberB(valueB), memberA(valueA) {}
- Use 'struct' for 'data-only' types
- Structs should _never_ have public/private/protected declarations, if there's anything non-public you shouldn't use a struct.
- Likely only going to be used within data reading/writing to files
- Because of this you're probably going to need to use 'sized' typed (see <cstdint> header)
struct DataFileSection { uint32_t x; uint32_t y; uint32_t z; };
- If using a struct to read in data, use a static_assert to ensure correct sizing:
static_assert(sizeof(DataFileSection) == 12, "DataFileSection not expected 12 bytes");
- If ownership of a member is tied to the class, don't use a pointer and new/delete in constructor/destructor. Just use the type and initialise it correctly in the init list before the constructor.
- If the above is not possible (e.g. complex init requirements, 'may be invalid and null' use a up<>
- If we /know/ a member reference owned by another object will be live for the duration of the class, use a &reference member
- Otherwise use a sp<>/up<> depending if it should take a reference and if having a 'null' object makes sense.
Functions
- Const functions where possible (IE not modifying any members)
class MyClass { public: int dataAccessor() const; };
- Const params where logically not to be modified
void function(const Type& readOnlyParam)
- Const returns where the caller should never modify
const Type& functionWhereYouCantModifyMyReturnThanks()
General code
- Where possible use auto
auto varableName = function();
- Note where auto& may be better to avoid a copy
- sp<> up<> wp<> smart pointers
- make_shared() instead of new
auto var = std::make_shared<Type>(args);
- make_unique() would be nice, but a c++14 feature (may restrict compiler compatibility?), so you have to wrap
- Use std::move to transfer up<> ownership
- make_unique() would be nice, but a c++14 feature (may restrict compiler compatibility?), so you have to wrap
up<Type> var{new Type{args}}; functionThatTakesOwnershipOfParam(std::move(var));
- Never use a 'naked' new - they should always be packaged immediately in a smart pointer
- Use emplace() in STL containers where possible over insert()
- Unless you explicitly want to copy an object in
- Use foreach loops where possible ( "for (auto &element : container)")
for (auto &element : container) { whatever(element); }
- Exception may be a 'safe' iterator when possibly removing elements during loop, then use iterator and keep copy locally
- Where possible use 'enum class'
- Naming variables - don't be afraid of using 'short' names ('i') if it's use is obvious
- While 'descriptive' names are nice, shorter code is also nice. Don't repeat context
- 'x' is fine is we already know we're doing something in coordinate space, no need to name it theXCoordinateOfTheCityMapInTiles
- Reading code is important - try to make it flow
- avoid 'yoda conditionals' (1 == var) don't help, modern compilers catch a =/== typo easily
- if post increment (x++) flows better use that, don't try to 'optimise' away the copy - the compiler will do that for you
- The compiler is more clever than you could ever possibly hope to be. Write things to be clear and obvious. Only /after/ it's proven to be a problem to you even look at optimisation (then _always have numbers_)
- don't use 'c' casts ("(int)x") - that does different things on if the object type has a defined conversion or not - use static_cast<>/reinterpret_case<> instead
- prefer {} constructor calls where possible
MyClass classInstance{argumentOne, argTwo};
- Requires you to avoid implicit conversions - this is good!
- STL initialiser lists are good
std::vector<int> someInts = {1, 2, 3};
- static_assert() any assumptions WRT alignment/packing (when reading structs from files, for example) - or any template restrictions (e.g. if this is only valid on unsigned types, check it!)
- <limits> is preferred to 'c' MAX_INT/whatever
auto maxInt = std::numeric_limits<int>::max();
- RAII where possible
- Early-return is cool, go ahead
if (dontHaveToDoAnything) { return; } doLotsOfBoringStuff();
- goto: is useful in some specific cases (e.g. breaking out of nested loops) - but only use it where another keyword won't do what you want
- Note limitations WRT goto: over stack initialisers - IE you can't do it :)
for (auto &x : containerX) { for (auto &y : x.containerY) { if (weShouldStopAt(y)) { goto end; } } } end: return;
Logging
- LogInfo/LogWarning/LogError take 'printf-style' format string. Make sure everything is the correct type, %d for ints, %u for unsigned, %s for "utf8" const char* "strings" or UString objects directly
- LogInfo is cheap - use it everywhere interesting
- LogWarning should be something that has gone wrong, but recoverable.
- LogError is for fatal errors.
Comments:
- either // or /* */ is fine - prefer // for single line
- If doing multi line /\*-style comments have an aligned '\*' with at the beginning of each subseqent line:
/* first line * second line * last line */
- Don't comment for the sake of it
- Try to make the code clearer first if a comment is 'required' to make something obvious
- Function/variable names are useful here - if reading it aloud describes what your comment was going to say that's perfect
- //TODO: //FIXME: when leaving known breakage
Includes
- "local.h" files first
- then <system.h> includes
- Within each of the 2 blocks try to keep them alphabetically sorted (some exceptions may happen, if there's a system dependency not managed by the system header itself)
- local headers always relative to the root of OpenApoc - even if in the same directory
- e.g. "framework/event.h" not "../event.h" or "event.h"
Headers
- prefer "#pragma once" to "#ifndef __HEADER_NAME" include guards
- Headers should avoid #include "dependency.h" where possible
- do forward declaration of classes instead where possible
class ForwardDeclaredType; void someFunction(ForwardDeclaredType ¶m);
- 'not possible' includes templates, non-class types, superclasses, try building it without and see what fails