Difference between revisions of "CodingStandards"

From The Battle for Wesnoth Wiki
m (See also)
(C++ version)
 
(37 intermediate revisions by 15 users not shown)
Line 1: Line 1:
Wesnoth uses modern/advanced C++ that is portable to Visual C++ 6 and GNU G++ 3.0+
+
Wesnoth uses C++ that is portable to C++ compilers targeting various commonly used platforms.
 +
 
 +
== C++ version ==
 +
 
 +
Wesnoth uses C++ conforming to C++11 in stable branch and C++14 in master branch.
  
 
== Formatting ==
 
== Formatting ==
Line 7: Line 11:
 
You may use long lines.
 
You may use long lines.
  
== Naming ==
+
Don't use a space after control flow keywords (if/for/...).
 +
 
 +
== Evil things to avoid ==
 +
 
 +
=== Avoid implicit conversions ===
 +
 
 +
Make all constructors which only take one argument that is of a different type to the class <tt>explicit</tt>.
 +
 
 +
Do not use <tt>operator T()</tt> (where <tt>T</tt> is a type) to allow an implicit conversion to a different type. For example:
 +
 
 +
t_string(const std::string&);
 +
 
 +
This can cause many situations where a temporary t_string is implicitly created and then gets destroyed unexpectedly (reference [https://gna.org/bugs/index.php?20360 bug #20360]).
 +
 
 +
=== Do not declare class data members as non-private ===
 +
 
 +
It's okay to have a ''struct'' with only public members, if that's what you want.
 +
 
 +
However, once something is a ''class'' with private data members, do not add public (or even protected) data members to the class. Doing this breaks encapsulation and can cause all kinds of confusing and evil things to happen.
 +
 
 +
=== Destructors must not throw exceptions ===
  
=== End Non-Public Members of Classes with an Underscore ===
+
Do not allow exceptions to propogate from a destructor. Doing that is always bad in C++. Any code which does it should be treated as a bug and fixed. Doing this is a very easy way to cause memory leaks and crashes.
  
All non-public data members of classes should have their names terminated with an underscore, to show that they are a
+
It's okay to have exceptions thrown inside a destructor, just as long as you catch() them inside the destructor too and don't allow them to propagate out.
class member. This makes for more readable code, once one is familiar with the convention.
 
  
== Idioms ==
+
In C++11, any destructor which throws an exception causes the program to be immediately terminated (except in special cases).
  
=== Use References when a value may not be NULL ===
+
You can read more about the issue here: http://c2.com/cgi/wiki?BewareOfExceptionsInTheDestructor
  
If a value passed to a function can never be NULL, use a reference instead of a pointer. I.e.
+
== Naming ==
  
  void myfunction(Object& obj);
+
=== End non-public class data members with an underscore ===
  
rather than
+
All non-public data members of classes should have their names terminated with an underscore, to show that they are a class member. This makes for more readable code, once one is familiar with the convention.
  
  void myfunction(Object* obj);
+
== Idioms ==
  
This more clearly shows the user of the function that obj may never be NULL,
+
=== Use references when a value may not be NULL ===
without them having to consult documentation or the implementation of the function.
 
  
=== Use Const ===
+
If a value passed to a function can never be <tt>NULL</tt>, use a reference instead of a pointer. For example:
  
The 'const' feature of C++ allows interfaces to more clearly specify how they treat objects.
+
void my_function(T& obj);
Always use const when you are not going to modify an object.
 
  
I.e.
+
rather than
  
  void myfunction(const Object& obj);
+
void my_function(T* obj);
  
demonstrates to the caller of myfunction() that obj will not be modified.  
+
This more clearly shows prospective users of the function that <tt>obj</tt> may never be <tt>NULL</tt>, without them having to consult documentation or the implementation of the function.
If myfunction may modify obj, then use
 
  
  void myfunction(Object& obj);
+
=== Use the const keyword ===
  
likewise, if a variable is not changed after initialization, make it const.
+
The <tt>const</tt> keyword in C++ allows interfaces to more clearly specify how they treat objects.
 +
Always use <tt>const</tt> when you are not going to modify an object. For example:
  
=== Write Exception-Safe Code ===
+
void my_function(const T& obj);
  
Wesnoth code should be exception-safe, even if you do not use exceptions directly.
+
This shows to the caller that <tt>obj</tt> will not be modified. If <tt>my_function()</tt> may modify <tt>obj</tt>, then use the following instead:
That is, you should be able to assume that an exception is thrown almost anywhere
 
from within the code, with well-defined results (i.e. no resource leaks).
 
  
Code that uses a pattern like,
+
void my_function(T& obj);
  
  {
+
Likewise, if a variable is not changed after initialization, make it <tt>const</tt>, and mark member functions as <tt>const</tt> if they do not modify their object.
  SDL_Surface* image = IMG_Load("image.bmp");
 
  ...some code, which uses 'image'...
 
  SDL_FreeSurface(image);
 
  }
 
  
is bad, because the code may throw an exception, and 'image' will never be freed.
+
=== Know the behavior of const references when types differ ===
Instead, use wrapper objects which free the object in their destructor.
 
  
For SDL_Surface objects, use the <tt>surface</tt> class.  
+
If you assign something to a <tt>const</tt> reference of a different type, if necessary (if the type is different but there is a conversion) the compiler will create a temporary and guarantee it lasts for the lifetime of the reference. So
So you could rewrite the above code,
 
  
  {
+
char c = 0;
  surface image(IMG_Load("image.bmp"));
+
const int& i = c;
  ...some code, which uses 'image'...
+
c = 5;
  } ''the image is automatically freed here when 'image' is destroyed
 
  
Instead of allocating memory directly using new[] or malloc(),  
+
will result in c == 5 and i == 0, which may not be what you expect.
use language-provided containers, such as vector.
 
  
=== Do not use sprintf ===
+
=== Write exception-safe code ===
  
Sprintf does not check whether or not it is writing past the end of the space allocated.  
+
Wesnoth code should be exception-safe, even if you do not use exceptions directly. That is, you should be able to assume that an exception is thrown almost anywhere from within the code, with well-defined results (i.e. no resource leaks).
This is a security problem if someone other than the person running the game
 
can cause sprintf to write very long strings.
 
In Wesnoth this untrusted data could come potentially from other players
 
in a multiplayer game or from downloaded campaigns.
 
Instead you should use snprintf with the second argument being sizeof of the buffer
 
that will hold the result.
 
  
== Standard C++ to avoid ==
+
Code that uses a pattern like the following is bad:
  
=== Respect for loop scoping of different platforms ===
+
{
 +
    SDL_Surface* image = IMG_Load("image.bmp");
 +
    ...some code, which uses 'image'...
 +
    SDL_FreeSurface(image);
 +
}
  
In the code,
+
The code may throw an exception, and <tt>image</tt> will never be freed. Instead, use wrapper objects which free the object in their destructor.
  
  for(int i = 0; i != 100; ++i) {...}
+
For <tt>SDL_Surface</tt> objects, the <tt>surface</tt> type is used throughout the Wesnoth source code to achieve this purpose. So you could rewrite the above code as follows:
  
the variable 'i' is scoped within the for loop according to ISO/ANSI C++ and GNU G++.  
+
{
However it is scoped within the surrounding scope according to Visual C++ 6.
+
    surface image(IMG_Load("image.bmp"));
 +
    ...some code, which uses 'image'...
 +
} ''the image is automatically freed here when 'image' is destroyed
  
This means that the code,
+
Instead of allocating memory directly using <tt>new[]</tt> or <tt>malloc()</tt>, use language-provided containers, such as vector.
  
  for(int i = 0; i != 100; ++i) {}
+
Similarly, avoid writing <tt>delete</tt> explicitly in your code. Oftentimes using a smart pointer or similar instead will improve the readability of your code, by making it obvious that you are managing memory correctly even if an exception is thrown. If you were deleting a pointer which is a member variable of an object, using a smart pointer instead may simplify your code by eliminating the need to write an explicit destructor in some cases.
  for(int i = 0; i != 100; ++i) {}
 
  
is illegal on VC++6, because i is defined twice,
+
For more information, you can read about the (important) RAII idiom: http://c2.com/cgi/wiki?ResourceAcquisitionIsInitialization
although it is legal according to the standard, and GNU G++.
 
  
On VC++6, the legal way to write it would be,
+
=== Do not use sprintf ===
  
  for(int i = 0; i != 100; ++i) {}
+
The <tt>sprintf()</tt> function does not check whether or not it is writing past the end of the space allocated.  This is a security problem if someone other than the person running the game can cause <tt>sprintf()</tt> to write very long strings. In Wesnoth, this untrusted data could come potentially from other players in a multiplayer game, or from downloaded add-ons. Instead you should use <tt>snprintf()</tt> with the second argument being the <tt>sizeof</tt> of the buffer that will hold the result.
  for(i = 0; i != 100; ++i) {}
 
  
But this is illegal according to the standard, because 'i' is not defined in the second loop.
+
== Standard C++ to avoid ==
The correct way to write this code to conform to the standard and work on all platforms
 
is to simply abandon declaring variables in the initialization statement of a for loop
 
when the variable must be reused in the same scope,
 
  
  int i;
+
=== Do not use 0 or NULL when you mean nullptr ===
  for(i = 0; i != 100; ++i) {}
 
  for(i = 0; i != 100; ++i) {}
 
  
=== Do not use wstring ===
+
Several Wesnoth developers, including Dave, find the number 0 to be very ambiguous when used in a non-numeric context. In keeping with the precedent that has already been established in the Wesnoth source code, you should avoid using literal zero for initializing and/or comparing null pointers.
  
The standard C++ wstring class, defined as a basic_string< wchar_t >,
+
=== Do not use standard io functions ===
does not exist in some platforms supported by Wesnoth.  
+
This implies the std::fstream class and fopen. These functions do not support utf8 in windows which is our standard encoding for filepaths. Use our custom filesystem functions (filesystem.hpp) instead.
Use wide_string, defined in language.hpp, instead.  
 
wide_string is actually defined as a vector< wchar_t >
 
  
 
== C legacy to be avoided ==
 
== C legacy to be avoided ==
  
=== Use the function templates minimum and maximum ===
+
=== Use std::array instead of C-style Arrays ===
  
Standard C++ offers the function templates min and max to find the minimum and maximum
+
C-style arrays are very efficient, but their interface is ugly. Use <tt>std::array</tt> instead.
of two values on which operator
 
<
 
is defined.  
 
Unfortunately, many hoops must be leapt through to get this working on VC++.
 
So, we do not use standard min and max.
 
Instead, we use minimum and maximum, defined in utils.hpp.
 
  
Usage is fairly natural:
+
=== Do not use C-style or function-style casts ===
  
  int i = minimum(x,y);
+
The following code,
  
Note that in the above example, if x is an unsigned integer, and y is a signed integer,
+
if(i->second.side() == (size_t)player_number_) {
VC++ will have problems.  
 
You must explicitly specify the version of minimum being called in such cases:
 
  
  int i = minimum<int>(x,y);
+
is considered bad practice in C++ since a C-style cast is overpowered -- if types change around it could end up casting away constness, or performing an implementation-defined data reinterpretation (basically a C-style cast is a compiler-generated combination of <tt>static_cast</tt>, <tt>reinterpret_cast</tt>, and <tt>const_cast</tt>).
  
=== Use util::array instead of C-style Arrays ===
+
This syntax,
  
C-style arrays are very efficient, but their interface is ugly.
+
if(i->second.side() == size_t(player_number_)) {
Use util::array defined in array.hpp.
 
It is a wrapper for an array which has a C++ container-style interface.
 
If you need to, extend it to make it fit your needs.
 
  
=== Do not use C-style casts ===
+
is also a cast, and [http://stackoverflow.com/a/32224 semantically identical] to the C-style cast. It should also be avoided.
  
The following code,
+
Good programming style is to use the least powerful tool available that does what you want. For example:
  
  if(i->second.side() == (size_t)player_number_) {
+
if(i->second.side() == static_cast<size_t>(player_number_)) {
  
is considered bad practice in C++ since a C-style cast is overpowered -- if types change around it could end up casting away constness, or performing an implementation-defined data reinterpretation (basically a C-style cast is a compiler generated combination of static_cast, reinterpret_cast, and const_cast).
+
Alternatively, a constructor call may be used for non-built-in types.
  
Good programming style is to use the least powerful tool available that does what you want.  
+
''Note: there may be some obscure cases where a C-style cast is desirable, such as converting a pointer to an integer type of unspecified size.''
For example,
 
  
  if(i->second.side() == static_cast<size_t>(player_number_)) {
+
=== Do not use #define for constants ===
  
Alternatively, a constructor call may be used for non-builtin types.
+
<tt><nowiki>#</nowiki>define foo X</tt> is not a typesafe approach to define constants. Instead, you can something like the following (in an anonymous namespace) to achieve the same goal in a typesafe fashion.
  
Note: there may be some obscure cases where a C-style cast is desirable,
+
namespace {
such as converting a pointer to an integer type of unspecified size.
+
    const T foo = X;
 +
}
  
 
== Documentation ==
 
== Documentation ==
  
=== Document "config" preconditions and postconditions ===
+
=== Document config preconditions and postconditions ===
  
In the Wesnoth code you will commonly encounter a data container known as the "config",  
+
In the Wesnoth code you will commonly encounter a data container type known as <tt>config</tt>, which contains hierarchical string data (such as WML contents or game settings). The tagged ''children'' of the <tt>config</tt> object and their string ''attributes'' are arranged in an ordered and mapped format, internally implemented using the C++ STL.
which contains heirarchical string data (such as WML contents or game settings).  
 
The tagged "children" of the config and their string "attributes" are arranged  
 
in an ordered and mapped format internally using STL.
 
  
Because config data is utilized in so many ways and places, it can be difficult to track across the scope of the entire program. You should document all public functions that take/return a config, specifying config content expectations (and updating any related entries in the [[ReferenceWML]] wiki pages).  
+
Because <tt>config</tt> data is utilized in so many ways and places, it can be difficult to track across the scope of the entire program. Thus, you should document all public functions that take/return <tt>config</tt> objects, specifying content expectations and updating any related entries in the [[ReferenceWML]] wiki pages. In particular, if your function requires a <tt>config</tt> parameter, specify where/how the <tt>config</tt> object should be created. This will be a great help to any future coders who need to call or modify your function.
In particular, if your function requires a config parameter, specify where/how the config should be created. This will be a great help to any future coders who need to call or modify your function.
 
  
 
=== Doxygen ===
 
=== Doxygen ===
See [[Doxygen]] for tips on how to comment the code,
+
 
so that doxygen can nicely document it.
+
See [[Doxygen]] for tips on how to comment the code, so that Doxygen can nicely document it.
  
 
== See also ==
 
== See also ==
 +
 
* [[HackingWesnoth]]
 
* [[HackingWesnoth]]
  
[[Category:Committers]]
+
[[Category:Development]]

Latest revision as of 21:17, 24 January 2019

Wesnoth uses C++ that is portable to C++ compilers targeting various commonly used platforms.

C++ version

Wesnoth uses C++ conforming to C++11 in stable branch and C++14 in master branch.

Formatting

When working on C++ for Wesnoth, indent your code with a tab character. After fully indenting, if you still need to line up the text with a specific character on the line above, you may further align it using space characters.

You may use long lines.

Don't use a space after control flow keywords (if/for/...).

Evil things to avoid

Avoid implicit conversions

Make all constructors which only take one argument that is of a different type to the class explicit.

Do not use operator T() (where T is a type) to allow an implicit conversion to a different type. For example:

t_string(const std::string&);

This can cause many situations where a temporary t_string is implicitly created and then gets destroyed unexpectedly (reference bug #20360).

Do not declare class data members as non-private

It's okay to have a struct with only public members, if that's what you want.

However, once something is a class with private data members, do not add public (or even protected) data members to the class. Doing this breaks encapsulation and can cause all kinds of confusing and evil things to happen.

Destructors must not throw exceptions

Do not allow exceptions to propogate from a destructor. Doing that is always bad in C++. Any code which does it should be treated as a bug and fixed. Doing this is a very easy way to cause memory leaks and crashes.

It's okay to have exceptions thrown inside a destructor, just as long as you catch() them inside the destructor too and don't allow them to propagate out.

In C++11, any destructor which throws an exception causes the program to be immediately terminated (except in special cases).

You can read more about the issue here: http://c2.com/cgi/wiki?BewareOfExceptionsInTheDestructor

Naming

End non-public class data members with an underscore

All non-public data members of classes should have their names terminated with an underscore, to show that they are a class member. This makes for more readable code, once one is familiar with the convention.

Idioms

Use references when a value may not be NULL

If a value passed to a function can never be NULL, use a reference instead of a pointer. For example:

void my_function(T& obj);

rather than

void my_function(T* obj);

This more clearly shows prospective users of the function that obj may never be NULL, without them having to consult documentation or the implementation of the function.

Use the const keyword

The const keyword in C++ allows interfaces to more clearly specify how they treat objects. Always use const when you are not going to modify an object. For example:

void my_function(const T& obj);

This shows to the caller that obj will not be modified. If my_function() may modify obj, then use the following instead:

void my_function(T& obj);

Likewise, if a variable is not changed after initialization, make it const, and mark member functions as const if they do not modify their object.

Know the behavior of const references when types differ

If you assign something to a const reference of a different type, if necessary (if the type is different but there is a conversion) the compiler will create a temporary and guarantee it lasts for the lifetime of the reference. So

char c = 0;
const int& i = c;
c = 5;

will result in c == 5 and i == 0, which may not be what you expect.

Write exception-safe code

Wesnoth code should be exception-safe, even if you do not use exceptions directly. That is, you should be able to assume that an exception is thrown almost anywhere from within the code, with well-defined results (i.e. no resource leaks).

Code that uses a pattern like the following is bad:

{
    SDL_Surface* image = IMG_Load("image.bmp");
    ...some code, which uses 'image'...
    SDL_FreeSurface(image);
}

The code may throw an exception, and image will never be freed. Instead, use wrapper objects which free the object in their destructor.

For SDL_Surface objects, the surface type is used throughout the Wesnoth source code to achieve this purpose. So you could rewrite the above code as follows:

{
    surface image(IMG_Load("image.bmp"));
    ...some code, which uses 'image'...
} the image is automatically freed here when 'image' is destroyed

Instead of allocating memory directly using new[] or malloc(), use language-provided containers, such as vector.

Similarly, avoid writing delete explicitly in your code. Oftentimes using a smart pointer or similar instead will improve the readability of your code, by making it obvious that you are managing memory correctly even if an exception is thrown. If you were deleting a pointer which is a member variable of an object, using a smart pointer instead may simplify your code by eliminating the need to write an explicit destructor in some cases.

For more information, you can read about the (important) RAII idiom: http://c2.com/cgi/wiki?ResourceAcquisitionIsInitialization

Do not use sprintf

The sprintf() function does not check whether or not it is writing past the end of the space allocated. This is a security problem if someone other than the person running the game can cause sprintf() to write very long strings. In Wesnoth, this untrusted data could come potentially from other players in a multiplayer game, or from downloaded add-ons. Instead you should use snprintf() with the second argument being the sizeof of the buffer that will hold the result.

Standard C++ to avoid

Do not use 0 or NULL when you mean nullptr

Several Wesnoth developers, including Dave, find the number 0 to be very ambiguous when used in a non-numeric context. In keeping with the precedent that has already been established in the Wesnoth source code, you should avoid using literal zero for initializing and/or comparing null pointers.

Do not use standard io functions

This implies the std::fstream class and fopen. These functions do not support utf8 in windows which is our standard encoding for filepaths. Use our custom filesystem functions (filesystem.hpp) instead.

C legacy to be avoided

Use std::array instead of C-style Arrays

C-style arrays are very efficient, but their interface is ugly. Use std::array instead.

Do not use C-style or function-style casts

The following code,

if(i->second.side() == (size_t)player_number_) {

is considered bad practice in C++ since a C-style cast is overpowered -- if types change around it could end up casting away constness, or performing an implementation-defined data reinterpretation (basically a C-style cast is a compiler-generated combination of static_cast, reinterpret_cast, and const_cast).

This syntax,

if(i->second.side() == size_t(player_number_)) {

is also a cast, and semantically identical to the C-style cast. It should also be avoided.

Good programming style is to use the least powerful tool available that does what you want. For example:

if(i->second.side() == static_cast<size_t>(player_number_)) {

Alternatively, a constructor call may be used for non-built-in types.

Note: there may be some obscure cases where a C-style cast is desirable, such as converting a pointer to an integer type of unspecified size.

Do not use #define for constants

#define foo X is not a typesafe approach to define constants. Instead, you can something like the following (in an anonymous namespace) to achieve the same goal in a typesafe fashion.

namespace {
    const T foo = X;
}

Documentation

Document config preconditions and postconditions

In the Wesnoth code you will commonly encounter a data container type known as config, which contains hierarchical string data (such as WML contents or game settings). The tagged children of the config object and their string attributes are arranged in an ordered and mapped format, internally implemented using the C++ STL.

Because config data is utilized in so many ways and places, it can be difficult to track across the scope of the entire program. Thus, you should document all public functions that take/return config objects, specifying content expectations and updating any related entries in the ReferenceWML wiki pages. In particular, if your function requires a config parameter, specify where/how the config object should be created. This will be a great help to any future coders who need to call or modify your function.

Doxygen

See Doxygen for tips on how to comment the code, so that Doxygen can nicely document it.

See also

This page was last edited on 24 January 2019, at 21:17.