Coding Style (OpenApoc)

The printable version is no longer supported and may have rendering errors. Please update your browser bookmarks and please use the default browser print function instead.

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 [1]. 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 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

	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 &param);

- 'not possible' includes templates, non-class types, superclasses, try building it without and see what fails