Permalink
Find file Copy path
Fetching contributors…
Cannot retrieve contributors at this time
530 lines (497 sloc) 15.7 KB

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 [http://llvm.org/releases/4.0.0/tools/clang/docs/ClangFormat.html]. 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