CODE_STYLE.md

Openapoc Coding Style Guidelines

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 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 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!)
• 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