Difference between revisions of "CodingStandards"

From The Battle for Wesnoth Wiki
(Doxygen)
Line 33: Line 33:
 
   void myfunction(Object* obj);
 
   void myfunction(Object* obj);
  
This more clearly shows the user of the function that obj may never be NULL, without them having to consult
+
This more clearly shows the user of the function that obj may never be NULL,  
documentation or the implementation of the function.
+
without them having to consult documentation or the implementation of the function.
  
 
=== Use Const ===
 
=== Use Const ===
  
The 'const' feature of C++ allows interfaces to more clearly specify how they treat objects. Always use const when you
+
The 'const' feature of C++ allows interfaces to more clearly specify how they treat objects.  
are not going to modify an object.
+
Always use const when you are not going to modify an object.
  
 
I.e.
 
I.e.
Line 45: Line 45:
 
   void myfunction(const Object& obj);
 
   void myfunction(const Object& obj);
  
demonstrates to the caller of myfunction() that obj will not be modified. If myfunction may modify obj, then use
+
demonstrates to the caller of myfunction() that obj will not be modified.  
 +
If myfunction may modify obj, then use
  
 
   void myfunction(Object& obj);
 
   void myfunction(Object& obj);
Line 53: Line 54:
 
=== Write Exception-Safe Code ===
 
=== 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).
+
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,
 
Code that uses a pattern like,
Line 63: Line 66:
 
   }
 
   }
  
is bad, because the code may throw an exception, and 'image' will never be freed. Instead, use wrapper objects which free the object in their destructor.
+
is bad, because 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, use the <tt>surface</tt> class. So you could rewrite the above code,
+
For SDL_Surface objects, use the <tt>surface</tt> class.  
 +
So you could rewrite the above code,
  
 
   {
 
   {
Line 72: Line 77:
 
   } ''the image is automatically freed here when 'image' is destroyed
 
   } ''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.
+
Instead of allocating memory directly using new[] or malloc(),  
 +
use language-provided containers, such as vector.
  
 
=== Do not use sprintf ===
 
=== Do not use sprintf ===
  
Sprintf 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 campaigns. Instead you should use snprintf with the second argument being sizeof of the buffer that will hold the result.
+
Sprintf 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 campaigns.  
 +
Instead you should use snprintf with the second argument being sizeof of the buffer  
 +
that will hold the result.
  
 
== Standard C++ to avoid ==
 
== Standard C++ to avoid ==
Line 86: Line 98:
 
   for(int i = 0; i != 100; ++i) {...}
 
   for(int i = 0; i != 100; ++i) {...}
  
the variable 'i' is scoped within the for loop according to ISO/ANSI C++ and GNU G++. However it is scoped within the
+
the variable 'i' is scoped within the for loop according to ISO/ANSI C++ and GNU G++.  
surrounding scope according to Visual C++ 6.
+
However it is scoped within the surrounding scope according to Visual C++ 6.
  
 
This means that the code,
 
This means that the code,
Line 94: Line 106:
 
   for(int i = 0; i != 100; ++i) {}
 
   for(int i = 0; i != 100; ++i) {}
  
is illegal on VC++6, because i is defined twice, although it is legal according to the standard, and GNU G++.
+
is illegal on VC++6, because i is defined twice,  
 +
although it is legal according to the standard, and GNU G++.
  
 
On VC++6, the legal way to write it would be,
 
On VC++6, the legal way to write it would be,
Line 101: Line 114:
 
   for(i = 0; i != 100; ++i) {}
 
   for(i = 0; i != 100; ++i) {}
  
But this is illegal according to the standard, because 'i' is not defined in the second loop. The correct way to write
+
But this is illegal according to the standard, because 'i' is not defined in the second loop.  
this code to conform to the standard and work on all platforms is to simply abandon declaring variables in the
+
The correct way to write this code to conform to the standard and work on all platforms  
initialization statement of a for loop when the variable must be reused in the same scope,
+
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;
 
   int i;
Line 111: Line 125:
 
=== Do not use wstring ===
 
=== Do not use wstring ===
  
The standard C++ wstring class, defined as a basic_string< wchar_t >, does not exist in some platforms supported
+
The standard C++ wstring class, defined as a basic_string< wchar_t >,  
by
+
does not exist in some platforms supported by Wesnoth.  
Wesnoth. Use wide_string, defined in language.hpp, instead. wide_string is actually defined as a vector< wchar_t >
+
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 ==
Line 119: Line 134:
 
=== Use the function templates minimum and maximum ===
 
=== Use the function templates minimum and maximum ===
  
Standard C++ offers the function templates min and max to find the minimum and maximum of two values on which operator
+
Standard C++ offers the function templates min and max to find the minimum and maximum  
 +
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
+
is defined.  
min
+
Unfortunately, many hoops must be leapt through to get this working on VC++.  
and max. Instead, we use minimum and maximum, defined in utils.hpp.
+
So, we do not use standard min and max.  
 +
Instead, we use minimum and maximum, defined in utils.hpp.
  
 
Usage is fairly natural:
 
Usage is fairly natural:
Line 129: Line 146:
 
   int i = minimum(x,y);
 
   int i = minimum(x,y);
  
Note that in the above example, if x is an unsigned integer, and y is a signed integer, VC++ will have problems. You
+
Note that in the above example, if x is an unsigned integer, and y is a signed integer,  
must explicitly specify the version of minimum being called in such cases:
+
VC++ will have problems.  
 +
You must explicitly specify the version of minimum being called in such cases:
  
 
   int i = minimum<int>(x,y);
 
   int i = minimum<int>(x,y);
Line 136: Line 154:
 
=== Use util::array instead of C-style Arrays ===
 
=== Use util::array instead of C-style Arrays ===
  
C-style arrays are very efficient, but their interface is ugly. Use util::array defined in array.hpp. It is a wrapper
+
C-style arrays are very efficient, but their interface is ugly.  
for an array which has a C++ container-style interface. If you need to, extend it to make it fit your needs.
+
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 ===
 
=== Do not use C-style casts ===
Line 147: Line 167:
 
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).
 
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).
  
Good programming style is to use the least powerful tool available that does what you want. For example,
+
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_)) {
 
   if(i->second.side() == static_cast<size_t>(player_number_)) {
Line 153: Line 174:
 
Alternatively, a constructor call may be used for non-builtin types.
 
Alternatively, a constructor call may be used for non-builtin 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.
+
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.
  
 
== Documentation ==
 
== Documentation ==
Line 159: Line 181:
 
=== 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," 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.
+
In the Wesnoth code you will commonly encounter a data container known as the "config",  
 +
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). 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.
+
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).  
 +
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,
 
See [[Doxygen]] for tips on how to comment the code,
so that doxygen can make nicely document it.
+
so that doxygen can nicely document it.
  
 
== See also ==
 
== See also ==
 
 
* [[HackingWesnoth]]
 
* [[HackingWesnoth]]
  
 
[[Category:Programmers]]
 
[[Category:Programmers]]
 
[[Category:Committers]]
 
[[Category:Committers]]

Revision as of 13:12, 15 August 2007

Wesnoth uses modern/advanced C++ that is portable to Visual C++ 6 and GNU G++ 3.0+

Formatting

Indent with a tab character (width 4 characters), but align with spaces so tab width can change.

You may use long lines. (Could we change this to max 80 columns per line? --TuukkaH)

How to on Emacs?

M-x set-variable tab-width 4
M-x set-variable c-basic-offset 4

How to set alignment to use spaces?

Naming

End Non-Public Members of Classes 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. I.e.

 void myfunction(Object& obj);

rather than

 void myfunction(Object* obj);

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

Use Const

The 'const' feature of C++ allows interfaces to more clearly specify how they treat objects. Always use const when you are not going to modify an object.

I.e.

 void myfunction(const Object& obj);

demonstrates to the caller of myfunction() that obj will not be modified. If myfunction may modify obj, then use

 void myfunction(Object& obj);

likewise, if a variable is not changed after initialization, make it const.

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,

 {
 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. Instead, use wrapper objects which free the object in their destructor.

For SDL_Surface objects, use the surface class. So you could rewrite the above code,

 {
 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.

Do not use sprintf

Sprintf 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 campaigns. Instead you should use snprintf with the second argument being sizeof of the buffer that will hold the result.

Standard C++ to avoid

Respect for loop scoping of different platforms

In the code,

 for(int i = 0; i != 100; ++i) {...}

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.

This means that the code,

 for(int i = 0; i != 100; ++i) {}
 for(int i = 0; i != 100; ++i) {}

is illegal on VC++6, because i is defined twice, although it is legal according to the standard, and GNU G++.

On VC++6, the legal way to write it would be,

 for(int i = 0; i != 100; ++i) {}
 for(i = 0; i != 100; ++i) {}

But this is illegal according to the standard, because 'i' is not defined in the second loop. 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;
 for(i = 0; i != 100; ++i) {}
 for(i = 0; i != 100; ++i) {}

Do not use wstring

The standard C++ wstring class, defined as a basic_string< wchar_t >, does not exist in some platforms supported by Wesnoth. Use wide_string, defined in language.hpp, instead. wide_string is actually defined as a vector< wchar_t >

C legacy to be avoided

Use the function templates minimum and maximum

Standard C++ offers the function templates min and max to find the minimum and maximum 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:

 int i = minimum(x,y);

Note that in the above example, if x is an unsigned integer, and y is a signed integer, VC++ will have problems. You must explicitly specify the version of minimum being called in such cases:

 int i = minimum<int>(x,y);

Use util::array instead of C-style Arrays

C-style arrays are very efficient, but their interface is ugly. 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

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).

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-builtin 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.

Documentation

Document "config" preconditions and postconditions

In the Wesnoth code you will commonly encounter a data container known as the "config", 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). 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

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

See also