Difference between revisions of "Coding Conventions"

== Language features ==

== Language features ==

* C++ exceptions (throw/catch): Not all C++ compilers support these correctly (esp. on embedded systems), and exception support incurs a noticeable overhead in binary size.

* C++ exceptions (throw/catch): Not all C++ compilers support these correctly (esp. on embedded systems), and exception support incurs a noticeable overhead in binary size.

* Global C++ objects: Their constructors / destructors will not be called on certain targets, causing all kinds of bad problems and requiring ugly workarounds. (The GCC option -Wglobal-constructors helps finding code doing this.)

* Global C++ objects: Their constructors / destructors will not be called on certain targets, causing all kinds of bad problems and requiring ugly workarounds. (The GCC option -Wglobal-constructors helps finding code doing this.)

We are reviewing these decisions from time to time, but so far, in our estimation the drawbacks of using any of these outweigh the hypothetical advantages.

We are reviewing these decisions from time to time, but so far, in our estimation the drawbacks of using any of these outweigh the hypothetical advantages.

* ScummVM runs on both little endian (Windows, x86 Mac OS X, x86 Linux...) and big endian hosts (PowerPC Mac OS X, PlayStation 3 Linux...). So when writing data (think savegames) to files and reading it back again, you need to compensate for this. This is easily done by using the READ_ and WRITE_ macros from common/endian.h (like READ_LE_UINT32 or WRITE_BE_UINT16.) respectively the corresponding Stream class methods (like readUint32LE or writeUint16BE).

* ScummVM runs on both little endian (Windows, x86 Mac OS X, x86 Linux...) and big endian hosts (PowerPC Mac OS X, PlayStation 3 Linux...). So when writing data (think savegames) to files and reading it back again, you need to compensate for this. This is easily done by using the READ_ and WRITE_ macros from common/endian.h (like READ_LE_UINT32 or WRITE_BE_UINT16.) respectively the corresponding Stream class methods (like readUint32LE or writeUint16BE).

* Furthermore, some games existed in multiple versions, e.g. one for Windows and one for Mac OS. In that case, you may have to detect and distinguish these versions and employ different reading calls, to compensate for endian differences in the game data files.

* Furthermore, some games existed in multiple versions, e.g. one for Windows and one for Mac OS. In that case, you may have to detect and distinguish these versions and employ different reading calls, to compensate for endian differences in the game data files.

== Struct packing ==

== Struct packing ==

Do *not* assume that structs are stored in a certain way in memory -- the layout of a struct in memory can vary a lot between platforms, and on most modern systems it will almost never be "packed". If you absolutely must use packed structs, do not just use some #pragma or __attribute__ (as that is not portable either). Instead, do the following:

Do *not* assume that structs are stored in a certain way in memory -- the layout of a struct in memory can vary a lot between platforms, and on most modern systems it will almost never be "packed". If you absolutely must use packed structs, do not just use some #pragma or __attribute__ (as that is not portable either). Instead, do the following:

* Before the struct(s) you need to be packed, insert

* Before the struct(s) you need to be packed, insert

   #include "common/pack-start.h" // START STRUCT PACKING

   #include "common/pack-start.h" // START STRUCT PACKING

* After the struct(s) you need to be packed, insert

* After the struct(s) you need to be packed, insert

   #include "common/pack-end.h" // END STRUCT PACKING

   #include "common/pack-end.h" // END STRUCT PACKING

* At the "end" of each struct you need to be packed, insert the following between the closing } and the ; after it:

* At the "end" of each struct you need to be packed, insert the following between the closing } and the ; after it:

   PACKED_STRUCT

   PACKED_STRUCT

You may want to look at some files which already do that, e.g. look at <code>engines/scumm/boxes.cpp</code>

You may want to look at some files which already do that, e.g. look at <code>engines/scumm/boxes.cpp</code>

The second issue concerns problems causing engines to be non-reentrant. Consider a code snippet like this:

The second issue concerns problems causing engines to be non-reentrant. Consider a code snippet like this:

   bool foo(bool cond) {

   bool foo(bool cond) {

     static bool bar = false;

     static bool bar = false;

     return bar;

     return bar;

   }

   }

Suppose this function is part of engine quux. When a new game using this engine is started, foo(false) will initially always return false. After foo(true) has been called the first time, foo will always return true. Maybe foo(true) is called when the user/player solves a certain puzzle in the game, and maybe the foo() function is meant to test whether this puzzle has already been solved.

Suppose this function is part of engine quux. When a new game using this engine is started, foo(false) will initially always return false. After foo(true) has been called the first time, foo will always return true. Maybe foo(true) is called when the user/player solves a certain puzzle in the game, and maybe the foo() function is meant to test whether this puzzle has already been solved.

Now imagine the player uses the "return to launcher" feature; once back in the launcher, they restart the game, again using the quux engine.

Now imagine the player uses the "return to launcher" feature; once back in the launcher, they restart the game, again using the quux engine.

This is another important reason why global variables keeping state are dangerous. At the very least, you ''must'' re-init any global variables whenever your engine starts. Relying on one-time initialization assignments to global variables, as in

This is another important reason why global variables keeping state are dangerous. At the very least, you ''must'' re-init any global variables whenever your engine starts. Relying on one-time initialization assignments to global variables, as in

   static int myGlobal = 0;

   static int myGlobal = 0;

is ''not'' sufficient.

is ''not'' sufficient.

In addition, global variables are strongly discouraged; if they are to be used, then they ''must'' be accompanied by a comment that explains why this static variable is necessary, and why it cannot be replaced by something else, like typically a member variable of a suitable class / object. The comment should also indicate where the variable is (re)set when the engine launches. If no such comment is present, a generic comment of the following form shall be added to the variable declaration:

In addition, global variables are strongly discouraged; if they are to be used, then they ''must'' be accompanied by a comment that explains why this static variable is necessary, and why it cannot be replaced by something else, like typically a member variable of a suitable class / object. The comment should also indicate where the variable is (re)set when the engine launches. If no such comment is present, a generic comment of the following form shall be added to the variable declaration:

   // FIXME: non-const global var

   // FIXME: non-const global var

As a minor exception, if an existing engine uses many globals (due to not (yet) being objectified), it is permissible to use a single comment to document multiple globals at once.

As a minor exception, if an existing engine uses many globals (due to not (yet) being objectified), it is permissible to use a single comment to document multiple globals at once.