Skip to content

Commit

Permalink
Updated style guide to emphasize clang format and manual squashing po…
Browse files Browse the repository at this point in the history
…ssibility
  • Loading branch information
hhyyrylainen committed Feb 15, 2019
1 parent 17ebef5 commit 30fb773
Showing 1 changed file with 65 additions and 58 deletions.
123 changes: 65 additions & 58 deletions doc/style_guide.md
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
Code Style Guide
================

To maintain a consistent coding style, contributors should follow the rules
outlined on this page. This style guide is separated into four parts: common
To maintain a consistent coding style, contributors should follow the rules
outlined on this page. This style guide is separated into four parts: common
rules, rules specific for C++, rules specific for AngelScript and guidelines for
using git.

The style rules are intended to increase readability of the source code. The
most important rule of all is: **Use common sense**. If you have to break
some rules to make the code more readable (and not just for you, but for
The style rules are intended to increase readability of the source code. The
most important rule of all is: **Use common sense**. If you have to break
some rules to make the code more readable (and not just for you, but for
everyone who has to read your code), break it.

Common (Both C++ and AngelScript)
Expand All @@ -17,11 +17,11 @@ Common (Both C++ and AngelScript)
- Indentation is 4 spaces

- Names (that includes variables, functions and classes) should be descriptive.
Avoid abbreviations. Do not shorten variable names just to save key strokes,
Avoid abbreviations. Do not shorten variable names just to save key strokes,
it will be read far more often than it will be written.

- Variables and functions are camelCase with leading lower case. Classes are
CamelCase with leading upper case. Constants are CONSTANT_CASE with
- Variables and functions are camelCase with leading lower case. Classes are
CamelCase with leading upper case. Constants are CONSTANT_CASE with
underscores.

- Filenames are lower_case with underscores. The reason for this is
Expand All @@ -34,46 +34,47 @@ Common (Both C++ and AngelScript)
C++
---

- Format your code with [clang-format](clang_format.md) this is the
official formatting and most important thing to consider. If you
don't automatic checks on your code will fail.

- Macros are CONSTANT_CASE

- Header files end in .h, source files in .cpp

- Header files should begin with `#pragma once`. Old-style header
- Header files should begin with `#pragma once`. Old-style header
guards (with `ifdef`) are discouraged because they are very verbose and
the pragma is understood by all relevant compilers.

- Format your code with [clang-format](clang_format.md)

- Opening braces go in the same line as the control statement, closing braces
are aligned below the first character of the opening control statement

- Keep header files minimal. Ideally, they show only functions / classes that
are useful to other code. All internal helper functions / classes should be
declared and defined in the source file.

- All classes and their public and protected members should be documented by
doxygen comments in the header file. If the function's purpose is clear
from the name and its parameters (which should be the usual case), the
doxygen comments in the header file. If the function's purpose is clear
from the name and its parameters (which should be the usual case), the
comment can be very basic and only serves to shut up doxygen warnings about
undocumented stuff.

- Inline comments inside functions can and should be used to describe why
you wrote the function like this (and, sometimes more importantly, why you
didn't go another way).

- Member variables of classes are prefixed with \p m_. This is to
differentiate them from local or global variables when using their
- Empty lines are encouraged between blocks of code to improve readability.

- Member variables of classes are prefixed with \p m_. This is to
differentiate them from local or global variables when using their
unqualified name (without `this->`) inside member functions. The prefix can
be omitted for very simple structs if they don't have member functions and
serve only as data container.

(- When calling member functions from another member function, their names are
qualified with `this->` to differentiate them from global non-member
functions.)
- When calling member functions from another member function, their names may be
qualified with `this->` to differentiate them from global non-member
functions. This rule is not enforced.

- Function signatures are formatted like the clang-format options file makes them to be formatted

- For non-trivial classes that would pull in a lot of other headers, use the pimpl idiom to hide implem entation details and only include the ton of headers in the .cpp file.
- For non-trivial classes that would pull in a lot of other headers,
use the pimpl idiom to hide implementation details and only include
the ton of headers in the .cpp file.

```cpp
// In the header:
Expand All @@ -92,12 +93,12 @@ C++
~MyClass();

private:

struct Implementation;
std::unique_ptr<Implementation> m_impl;
};
```
```cpp
// In the source file:
Expand All @@ -112,24 +113,24 @@ C++
MyClass::~MyClass() {} // Define destructor
```

- Try to avoid include statements inside header files unless
absolutely necessary. Prefer forward declarations and put the
include inside the source file instead. And use the pimpl idiom if
this cannot be avoided and the headers are large.

- Prefer C++11's `using` over `typedef`. With the `using` keyword, type
- Try to avoid include statements inside header files. Prefer forward
declarations and put the include inside the source file instead. And
use the pimpl idiom if this cannot be avoided and the headers are
large.

- Use C++11's `using` over `typedef`. With the `using` keyword, type
aliases look more like familiar variable assignment, with no ambiguity as
to which is the newly defined type name.

- Virtual member functions overridden in derived classes are marked with the
C++11 `override` keyword. This will (correctly) cause a compile time error
when the function signature in the base class changes and the programmer
- Virtual member functions overridden in derived classes are marked with the
C++11 `override` keyword. This will (correctly) cause a compile time error
when the function signature in the base class changes and the programmer
forgot to update the derived class.

- Classes not intended as base classes are marked with the `final` keyword
like this:

```cpp
class MyClass final {
// ...
Expand All @@ -147,7 +148,7 @@ C++
#include <Entities/Component.h>
#include <Entities/System.h>
#include <OgreMatrix4.h>
#include <vector>
Expand All @@ -160,18 +161,16 @@ C++
AngelScript
-----------

- A class's public data members are *not* prefixed by `m_`, unlike C++.

(This is because in AngelScript, all member variables are accessed with their qualified
names (like `this.memberVariable`), so there is no need to mark them.)
- A class's public data members don't need to be prefixed by `m_`, unlike C++.

- A class's private data members and functions are declared `private`
(everything is public by default) and optionally prefixed with an
underscore. This is a convention adopted from Python's PEP8 style
guide.
guide. This same rule may also be used in C++.

- Doxygen does not support AngelScript natively, but for consistency's sake, AngelScript
classes and functions are still documented with doxygen style comments.
- Doxygen does not support AngelScript natively, but for consistency's
sake, AngelScript classes and functions are still documented with
doxygen style comments.

- For consistency with C++ adding semicolons after class declarations
is recommended, but not an error if omitted. This is one of the
Expand All @@ -181,9 +180,11 @@ AngelScript
Git
---

- Do not work in the master branch, always create a private feature branch
if you want to edit something

- Do not work in the master branch, always create a private feature
branch if you want to edit something. Even when working with a fork
this is recommended to reduce problems with subsequent pull
requests.

- If you don't have access to the main repository yet (you will be
granted access after your first accepted pull request) fork the
Thrive repository and work in your fork and once done open a pull
Expand All @@ -204,7 +205,10 @@ Git
- When the master branch is updated, you should usually keep your
feature branch as-is. If you really need the new features from
master, do a merge. Or if there is a merge conflict preventing your
pull request from being merged.
pull request from being merged. Quite often the master branch needs
to be merged in before merging a pull request. For this it is
cleaner if the pull request is rebased onto master, but this is not
required.

- When a feature branch is done, open a pull request on GitHub so that others
can review it. Chances are that during this review, there will still be
Expand All @@ -215,13 +219,16 @@ Git
done (take note people accepting pull requests) from the Github
accept pull request button, hit the arrow to the right side and
select "merge and squash". Bigger feature branches that are dozens
of commits from multiple people will be merged normally to attribute
all of the authors on Github feeds.

- For maintainers: When manually squashing GitHub (which is something
you should avoid) requires a merge commit to recognize the merging
of a pull request. A "git merge --squash" does not create a merge
commit and will leave the pull request's branch "dangling". To make
GitHub properly reflect the merge, follow the procedure outlined in
the previous bullet point, then click the "Merge Pull Request"
button on GitHub (or do a normal "git merge".
of commits or from multiple people need to be manually squashed into
a few commits while keeping commits from different authors separate
in order to attribute all of the authors on Github feeds. These kind
of pull requests can be merged normally **if** all the commits in them
are "clean" to not dirty up master.

- For maintainers: When manually merging (which is something you
should avoid) GitHub requires a merge commit to recognize the
merging of a pull request. A "git merge --squash" does not create a
merge commit and will leave the pull request's branch "dangling". To
make GitHub properly reflect the merge, follow the procedure
outlined in the previous bullet point, then click the "Merge Pull
Request" button on GitHub (or do a normal "git merge").

0 comments on commit 30fb773

Please sign in to comment.