OpenOffice.org Coding Guidelines

Draft. Many items missing or incomplete.

Authors:	Thorsten Behrens

Stephan Bergmann

Matthias Hütsch

Nikolai Pretzell

Tino Rachui

Jens-Heiner Rechtien

Hennes Rohling

et al.

Created: 2002-06-16

Last changed on 2004-02-26 by Stephan Bergmann

Abstract

We are uncovering better ways of developing
software by doing it and helping others do it.

―Manifesto for Agile Software Development
<http://www.agilemanifesto.org/>

This document provides guidelines and hints for general OpenOffice.org programming. We describe common pitfalls and the "Best Practices" to overcome them. It includes a collection of specific idioms and tips on certain APIs and contains a section on how to placate compilers on different levels of standard conformance.

Table of Contents

1	Introduction	4

2	Syntax Peculiarities	5

2.1	Basic Rules	5

2.2	Rules to avoid Breaks at Compile Time	6

2.3	Rules to avoid Breaks at Runtime	8

3	Local Conventions	11

3.1	Code defensive	11

3.2	Code readable	16

3.3	Anachronisms	18

3.4	Documentation	22

4	Coding Guidelines for C++ in General	23

4.1	General Principles	23

4.2	Class Design	24

4.3	Class Inheritance	30

4.4	Function Design	32

4.5	Const Correctness	35

4.6	Statement level idioms	35

4.7	Dependency Management	39

4.8	Header Inclusion	39

4.9	Documentation and Comments	39

5	Random Bag of Coding issues	41

5.1	Memory Visibility between Threads	41

5.2	Integral Types	42

5.3	Using the Observer Pattern	43

5.4	Optimization	47

5.5	Aesthetics	48

5.6	Refactoring	48

6	The Proper Usage of Selected Often Used APIs	50

6.1	Best Practices Using “sal” Interfaces.	50

6.2	Best Practices Using “osl” Interfaces	51

6.3	Best Practices Using “osl” File System Interface	53

7	Typical Problems within Office Coding	54

7.1	Byte Strings versus Unicode Strings	54

8	Java Coding Guidelines	55

9	Further Reading	56

Introduction

The OpenOffice.org code base is huge and often hard to understand for newcomers and for experienced OpenOffice.org coders as well. It has evolved over many years and has seen major changes in programming language standards and programming paradigms. These changes are reflected in a rich assortment of programming styles, idioms and conventions dependent on when a particular piece of code was introduced and to which layer it belongs. This document is meant to be a guideline on certain aspects of OpenOffice.org coding which we feel are of special importance. It's neither a tutorial nor a reference on OpenOffice.org programming (these can be found elsewhere) but rather a loose assortment of tips and recommendations.

Many of these "Best Practices" recommendations might be overly obvious to the experienced programmer. But then what is obvious to one developer might not be for the other. Most items in this document were prompted by actual problems we found in the code base or lessons we learned during the development of this large scale software product.

These recommendations are just that: recommendations. Use them with an open eye for their applicability. There might be reasons to do it different. That's okay. The only part of this document which covers mandatory items is the chapter about syntax requirements. It's hard to argue with compilers.

Syntax Peculiarities

OpenOffice.org has been ported to several compilers on a number of platforms. Sadly many current compilers are still not up to the latest C++ standard or exhibit some peculiarities which prevent the compilation of code fragments which are accepted by other compilers. Please stick to the rules in this chapter otherwise the build will break somewhere on someone's platform.

Basic Rules

Approved syntax

The following C++ language constructs should work on any supported compiler/platform combination. Use at will. Compilers which don't support these constructs will probably never compile OpenOffice.org.

mutable keyword

explicit keyword

templates, but not member templates (see below)

Disapproved syntax

Some C++ language constructs do not work with all supported compilers, so don't use:

member templates (MSVC++ 6 does not support member templates)

alternative logical keywords:

and

and_eq

bitand

bitor

compl

not

or

or_eq

xor

xor_eq

not_eq

some compilers do implement these keywords, some don't. It follows that you can neither use the words on this list as keywords nor as identifier.

export keyword (has any compiler ever implemented this right?)

Don't use C++ comments in C code or headers

Many compilers accept C++ (// ...) comments. Some don't. Use C comments (/* ... */ if you write C code or if you write header which are included from both C and C++ code.

Don't use non-ASCII characters

Never place characters for which the most significant bit is set into C/C++ files. If you need them, e.g. in strings, use the escape notation (“\” followed by the octal character index or “\x” followed by the hexadecimal index). Depending on your compiler, operating system and locale, your non-ASCII character might not even have valid code points and can trigger undefined behavior. This has already happened with MSVC++ on a Chinese Windows system.

Always conclude your source file with a new line character

The Forte compilers have a funny bug that will prevent correct parsing of the last line if a file does not end with a new line. This leads to some strange diagnostics and to very hard to find syntax errors. Real :-) editors will automatically add a line end on saving. If you use a lesser editor please add an empty line to the end of your file.

Rules to avoid Breaks at Compile Time

Always include all the STL header you need

Newer versions of the STLport are more modular than the venerable old STLport-4.0 we still use with the MSVC++ platforms. Especially <algorithm> and <functional> are no longer indirectly included by the container header <vector>, <list>, <map> etc. Just include everything you need.

Use consistent exception specifications

GCC-3.x.y verifies exception specifications over inheritance hierarchies. The rule is, that an overwritten method in a derived class can't have a looser exception specification than the method in the base class. This also means that if the base class method has an exception specification the derived class method must also have an exception specification.

MSVC++ ignores exception specifications completely, Forte 6 update 1 (unpatched) verifies only the consistency of exception specifications between declaration and definition. Newer patched versions of Forte 6 update 1 behave like GCC-3.x.y.

It's easy to overlook these inconsistencies, but please make certain that they do not happen. Nothing will raise the blood pressure of release engineers faster than inconsistent exception specifications :-)

Non const reference method parameters

GCC has a very strict idea of what is a valid initializer for a "plain" (non const) reference.

Please consider the following (wrong) code fragment:

class Foo

{

...

};

Foo getfoo()

{

...

return Foo();

}

void usefoo(Foo& aFoo)

{

...

}

void f()

{

...

usefoo(getfoo());

....

}

This fragment won't compile because the temporary in the call of usefoo() is not an “lvalue", that is something you can take the address of.

Either use a “const Foo&“ as parameter to usefoo() or, if this is not possible, give the temporary a name:

void f()

{

...

Foo aTmpFoo(getfoo());

usefoo(aTmpFoo);

...

}

Please note that this is not a bug in GCC. Stroustrup's "The C++ Programming Language", 3rd edition, on page 98 says:

"The initializer for a "plain" T& must be an lvalue of type T."

And on page 99 it says:

"The semantics of argument passing are defined to be those of initialization, [...]"

In about 80% of the cases where release engineering had to add a named temporary to pacify GCC, an interface change from a "Foo&" parameter to a "const Foo&" parameter would have been possible and thus better (see the item about "const correctness").

GCC can't parse simple code?

GCC sometimes fails to compile simple code like this:

struct A {

A();

A(int);

};

struct B {

B();

B(int);

};

struct C {

C(A);

C(A, B);

};

void f()

{

C c(A(), B(1));

}

$ g++ -c fail.cxx

fail.cxx: In function `void f()':

fail.cxx:18: parse error before numeric constant

This is a very well known parsing bug which hasn't been fixed in years - so it must be really hard to fix. It's even documented in the GCC manual.

Unfortunately we have an often used class combination which triggers this kind of error: Rectangle, Point and Size.

The construct:

#include <tools/gen.hxx>

...

Rectangle aRectangle(Point(), Size(400,300));

fails to compile with the above mentioned parse error. A slight change will convince GCC to compile the line:

Rectangle aRectangle(Point(0,0), Size(400,300));

This is not the only case where this GCC bug hits us, but it's by far the most common.

For-Scope

TBD.

Always include <sal/config.h> as the very first header

In every file that is thrown at the compiler, including other headers and implementation files, a

#include <sal/config.h>

should be the first non-comment, non-whitespace statement. The config header portably sets up a proper environment, possibly enabling/disabling compiler features, warnings and errors via #pragmas, or defining/undefining macros that would otherwise interfere with the remainder of the OpenOffice coding guidelines.

One prominent example why to include <sal/config.h> even in header files where no OOo types nor other specialities are used is the following: the Solaris platform defines the macro 'sun', somewhere within the compiler and/or implicitely included header files. Unfortunately, with UNO, 'sun' is a very common namespace component, which would be replaced by '1', without including <sal/config.h>.

Rules to avoid Breaks at Runtime

Use proper array initialization statements

GCC-3.x.y is picky about array initialization. Please consider the following (wrong) code fragment:

struct MyStruct {

union {

char *foo;

char *bar;

}

int nInt;

};

MyStruct aMyStruct[] = {

"xyz", 1,

"123", 2,

...

}

This kind of array initialization is wrong but used to give correct results on all compilers until GCC-3.0.1. The newer GCC compilers will issue lots of warnings (at least if warnings aren't switched off) and misaligns the array elements. At runtime indexes into the array will fetch the wrong elements.

Please use the proper initialization syntax with additional curly braces:

MyStruct aMyStruct[] = {

{ "xyz", 1 },

{ "123", 2 },

...

}

Always use matching new[] / delete[] statements

Sure, your basic C++ programming book says that you should do so. But you would be surprised how often this goes wrong. The problem is that it's sometimes hard to notice a mismatch. Please consider the following (wrong) code fragment:

char* ptr = new char[256];

...

delete ptr;

Some compilers will notice that ptr is a pointer to a plain data type and they know that they don't have to run 256 destructors upon deletion of ptr. In such a case the compiler can optimize the code to just allocate the raw memory, i.e. without allocating additional space for bookkeeping.

Well, the compiler may do this optimization but it is written nowhere that it has to do it. We found out the hard way that the GCC, Forte and MSVC++ compilers generate code that is not susceptible for heap corruption in such a case but the Tru64 compiler will happily free a pointer with 8 byte offset. Not good.

Always use matching new[] and delete[] statements:

char* ptr = new char[256];

...

delete [] ptr;

Avoid catch(...)

Due to long-lasting problems in the Solaris compiler, usage of the catch-all statement catch(...) is discouraged. If the methods you call have an exception specification, catch that exceptions (others call std::unexpected() anyway). If it has none, catching uno::Exception will probably deal with every error that can be dealt with. A possible problem are destructors, which are required not to throw. Try hard to avoid calling code that can throw in a destructor, because you have little to no leeway to deal with that error. If there is nontrivial work to be done on object destruction, try to move that into a finalize() method and call it beforehand.

Indication:

- everywhere (will break on Solaris)

Contraindication:

- nowhere (except on non-Solaris system specific code)

Examples:

- obvious

Be aware of problems with complex initializers of global objects.

If possible avoid global objects with complex initializers. This is especially true for file scope static arrays initialized with Unicode strings. Sometimes it works, sometimes not. The problem is, that it's not easy to predict the order of the execution of global initializers over C++ single object boundaries, not talking about the dozens of shared libraries involved in an office startup.

Local Conventions

Code defensive

Keep new projects -Wall clean

When creating new projects, strive to keep them free of compiler warnings. Currently, you get the most sophisticated warnings from GCC; check the following option line:

-Wall -Wfloat-equal -Weffc++ -Wold-style-cast -Woverloaded-virtual

-Wshadow -Wpointer-arith -Wcast-align -Wsign-compare -Winline

This enables common warnings plus some goodies including warnings for non-virtual destructors, differing default parameters for overridden virtual methods, shadowing of member variables, non-portable pointer arithmetic, including change of alignment requirements, comparison of signed and unsigned integer types and methods declared inline but outlined by the compiler.

You can set those flags on the environment variable ENVCFLAGS, to persuade dmake to use them correctly during compilation, you have to issue a "dmake wall=t"

Indication:

- on new projects

Contraindication:

- on old projects, you will get flooded with warnings; use it only selectively, e.g. when you have refactored larger parts

Examples:

See above.

Use header file include guards

Even more, have header file include guards also at the including place. Otherwise, most of our compilers will happily open and parse every header file again and again (the former is the problem, hitting network bandwidth and file server caches).

Indication:

- everywhere

Contraindication:

- nowhere

Examples:

#ifndef _OSL_MUTEX_HXX_

#include <osl/mutex.hxx>

#endif

Do not use namespace inclusions in header files

Never import namespaces or single identifiers in header files with the using directive, because, depending on which headers are included in which order, you might unknowingly create name clashes or ambiguities. The problem here is that somebody else changing header files that are somewhere included together with yours might unknowingly break a build.

Indication:

- nowhere

Contraindication:

- everywhere

Examples:

header1.hxx:

namespace A

{

class test;

}

header2.hxx:

class test;

header3.hxx

#include "header1.hxx"

using namespace A;

file.cxx

#include "header2.hxx"

#include "header3.hxx"

now class test is ambiguous.

Use at utmost one namespace inclusion per cxx

As especially UNO interfaces reside in quite deeply nested namespaces like com::sun::star::..., it is often convenient to include the whole namespace. That effectively puts all identifiers defined in that namespace into the top-level namespace. But refrain from doing that multiple times, since you risk name clashes or ambiguities. A better way then is to alias the long name to a shorter one.

Indication:

- everywhere

Contraindication:

- nowhere

Examples:

using namespace com::sun::star;

uno::Any aAny;

or even better

namespace css = com::sun::star;

css::uno::Any aAny;

Make liberate use of DEBUG macros

The header <tools/debug.hxx> provides a wealth of debug utilities, some of which are presented here (you can safely ignore the rest, they are either of very limited audience, deprecated or simply not working anymore). First of all, the relevant preprocessor define set in debug builds is DBG_UTIL:

#ifdef DBG_UTIL

CheckInvariants();

#endif

To avoid build problems (depending on DBG_UTIL, different code might be compiled), try to use the following idiom (at least if your compiler is not completely brain dead and does not optimize away dead code):

#ifdef DBG_UTIL

#define CHECK_INVARIANTS 1

#else

#define CHECK_INVARIANTS 0

#endif

if( CHECK_INVARIANTS )

{

CheckInvariants();

}

If your debug checks are simple predicates (e.g. for verifying input parameters), just use the DBG_ASSERT macro, which opens an error box if the given condition evaluates to false:

void MyClass::myMember( const char* pStr )

{

DBG_ASSERT( pStr != NULL, "MyClass::myMember: Invalid string pointer");

}

If you can recover from an error condition, you might consider DBG_ASSERTWARNING, which by default does not open an error box:

void MyClass::myMember( const char* pStr )

{

DBG_ASSERTWARNING( pStr != NULL, "MyClass::myMember: Invalid string pointer");

if( pStr != NULL )

{

...

}

}

To configure the application's debug subsystem, press <shift><control><d> in a window (debug builds only).

For unconditional error reporting, there exist the DBG_TRACE, DBG_WARNING and DBG_ERROR macros, which take a string argument each and write it to the debugger console, a warning window or into an error message box, respectively (there are FILE* variants of this macros, which put the error string into a file). In case you have to perform some string formatting in your debug output, there are DBG_TRACE1 – 5, DBG_WARNING1 - 5 and DBG_ERROR1 - 5, where you can supply a string and up to five arguments, similar to the printf() class of C functions:

DBG_WARNING2( "This object was %d times created and only %d times

deleted.", nCreations, nDestructions );

By the way, if you want to perform instance counting of objects, you are better off using the following set of macros:

In your class implementation file, at file scope, issue a

DBG_NAME(ClassName);

If you want to use DBG_CTOR, DBG_DTOR already in your header file (because they are inline), add a

DBG_NAMEEX(ClassName);

to declare some internal data structures. In every constructor (do not forget the copy constructor, if you don't declare it, the compiler will generate one!), place a

DBG_CTOR(ClassName, NULL);

and in the destructor a

DBG_DTOR(ClassName, NULL);

In every member, you can place a

DBG_CHKTHIS(ClassName, NULL);

which checks the validity of your class' this pointer.

And now you wonder what that NULL pointer is all about in the macros described above. This parameter can be a pointer to a function with the following signature:

const sal_Char* TestFunc(const void* pThis );

where pThis is a pointer to your object instance (yes, you have to static_cast it to the class' type), and the return value, if not NULL, is an error string. Thus, by providing this function pointer, you can perform e.g. invariant checks on your class' instances.

A somewhat special debug macro is

DBG_TESTSOLARMUTEX();

which checks whether the infamous “solar mutex” is locked (and issues an error if not). One useful place for it are e.g. helper methods or helper classes on UNO objects, where your contract specified that they must be called with a locked “solar mutex”.

Declare methods const as const can

When declaring methods (also on APIs), strive to make them as const as you can. Constness means always "logical" or "conceptual" constness, i.e. a method is "const" if it's call does not lead to changes to the object *that are detectable by it's clients*. This is in contrast to bitwise constness, which means that in const-methods the object's bit pattern in memory is not altered. It is easy to come up with examples where bitwise constness leads to unexpected behavior (just remember pointer members, if the pointee is part of the object's state).

Just because the method at hand modifies a member is not per se a reason to make it non-const, only if that method changes the object's state in a way visible to the caller. To calm down your compiler if you change member variables in a const method, declare those (and only those) mutable (this keyword is supported by all compilers in use).

A different, but also important area are method parameters. If a parameter is given by reference or pointer, and is input-only, it should be declared const. On the other hand, declaring by-value parameter const is not only superfluous, but might force a maintainer or implementer of your interface to copy by-value parameter to work with them. Don't do that.

Indication:

- everywhere

Contraindication:

- nowhere (except if you want your clients to curse you)

Examples:

Imagine you have a class that uses a very large object as a helper class, which is only needed from certain methods. A natural way is to hold that object as a pointer member and create it the first time one of those methods is called:

class Example

{

public:

...

int GetVal1() const;

int GetVal2() const;

private:

int mnVal1;

mutable Helper* mpHelper;

};

int Example::GetVal1() const

{

return mnVal1;

}

int Example::GetVal2() const

{

if( !mpHelper )

mpHelper = new Helper();

return mpHelper->GetVal();

}

This does compile, because mpHelper is declared mutable. This is correct, too, since mpHelper is in no way exposed to the clients.

There remains one subtlety inherent to pointer members. As a matter of fact, pointer (and references) can model two kinds of relationships: Either the pointee is part of the pointer holder's state, just as for ordinary member variables. A special case of this is the "contains" relation, where the object owns the pointee. Or the pointer models a "uses" relation, i.e. holding a pointer only for informational reasons, because the two objects e.g. cooperate. In the first case, constness should be transitive, i.e. calling non-const methods on the pointee should not be allowed within const methods of the pointer holder. This leads to identical behavior (and compiler errors) as for ordinary members. In the second case, constness should not be transitive, since the pointee does not belong to the object's state (if it does, you should check whether it also belongs to the object's visible state. If yes, make constness transitive). Normal C++ pointer and references model only the second case, but a simple template accounts for the first (proposed by Kevlin Henney in comp.lang.c++.moderated):

template<typename T>

class qualified_ptr

{

public:

T *operator->()	{ return mP; }

const T *operator->() const { return mP; }

...

private:

T *mP;

};

If in doubt, declare your methods const and make your pointer members const-transitive, until the compiler complains. Then check if it's your mistake or a case for mutable.

Always have a default case in your switch statement

TBD.

Try to use single exit points in your methods

Really? TBD.

Code readable

Use a consistent way to distinguish member and local variables

When naming your member variables, prefix or postfix them in a consistent way throughout your project. This avoids unintentional shadowing of local variables and makes your code more readable.

Indication:

- everywhere

Contraindication:

- nowhere

Examples:

Common ways for member variable naming schemes are:

mMember;

m_Member;

member_;

The recommended style is this: mMember

Prefix variables with a one-character type identifier

To enhance readability, it is common practice to prefix variable names with a one-letter identifier denoting it's type.

Indication:

- everywhere

Contraindication:

- nowhere, but refrain from Microsoft's lpszMyString ugliness

Examples:

for numbers, use n: nCounter

for strings, use s: sString

for characters, use c: cChar

for doubles and floats, use: fNum

for pointer, use p: pPtr

for interfaces, use x: xRef

for all the rest, use a: aAny

Employ a data-type centric variable declaration

When declaring pointer or reference variables, prefer the data-type centric notation:

sal_Int16* pMem; instead of sal_Int16 *pMem;

sal_Int16& pMem; instead of sal_Int16 &pMem;

This focuses on the data type of the variable pMem, putting all that is a part of that data type declaration on the left side.

Under all circumstances, avoid declaration of more than one pointer or reference variable per line:

sal_Int16 &pMem1, &pMem2; // DON'T DO THAT

since especially for references, it is quite easy to forget the second "&", get away with no compiler error but with a bug that's hard to spot.

Indication:

- everywhere

Contraindication:

- nowhere

Examples:

See above.

Anachronisms

Use the “sal” types instead of “solar” types

In <sal/types.h> the following basic types a defined (among others):

sal_Int8

sal_uInt8

sal_Int16

sal_uInt16

sal_Int32

sal_uInt32

sal_Int64

sal_uInt64

sal_Char

It is recommended to use those types not only in UNO interfaces, but also in normal code, with the following two notable exceptions: sal_Bool and sal_Unicode. With sal_Bool, you loose some of the type safety that bool provides, and you cannot overload on sal_uInt8 and sal_Bool simultaneously. The same applies to sal_Unicode, which might be a typedef to sal_uInt16.

For sal_Bool, simply use the builtin bool, except on UNO interfaces. For sal_Unicode, you have to be wary and e.g. explicitly specify the cppu type when setting them to uno::Anys.

A further note is in order on tight loops and performance: since the 16 bit platforms have been abandoned, an int is currently at least 32 bit wide. Thus, for loop variables where performance is an issue, you might consider using native ints: this is per definition the processor's most natural integer type, where arithmetic operations are fastest.

Also note that relying on the fact that addresses are 32 bit wide makes porting to 64 bit architectures unnecessary hard. Thus, when working with memory sizes or doing pointer arithmetic, use size_t as the integer data type.

Indication:

- sal_Bool only on interfaces

- sal_Unicode with caution

- the rest: everywhere

Contraindication:

- sal_Bool on overloaded methods

- the rest: nowhere

Examples:

To correctly put a sal_Unicode into an uno::Any, use the following idiom:

sal_Unicode cMyChar;

aAny.setValue( &cMyChar, ::getCppuCharType() );

sal_Unicode cMyChar;

uno::Any a( &cMyChar, ::getCppuCharType() );

Use try, catch and const_cast<> instead of TRY, CATCH etc.

In <vos/xception.hxx> and various other header files, upper-case macros for keywords added during the C++ standardization are declared. Since as of today, nowadays even the least standard compliant compiler (MSVC) handles the keywords try, catch, const_cast<>, static_cast<>, reinterpret_cast<> and dynamic_cast<> correctly, there is no need to clutter your code with those workaround-macros.

There is, although, one notable exception: <sal/types.h> declares the SAL_THROW macro, which should be used for exception specifications. This is because some compiler start to generate unnecessary ineffective and/or larger code, if exception specifications are given. On those platforms, SAL_THROW might be defined empty, to avoid that. If you need dynamic_cast<>, and thus RTTI, check whether it is not disabled in the local makefile (this is the case for most of the older code base).

Indication

- everywhere

Contraindication

- nowhere

Examples

- obvious

Use std::vector<> instead of Sv*Arrays

The header <svtools/svarray.hxx> defines various macros for declaring and using container with pre-standard compilers (i.e. without proper template support, such that STL cannot be used). Besides the fact that nowadays even the least standard compliant compiler (MSVC) can be used without hassles with the STL, these macros have some glitches and limitations. Among them is the problem that indices are only 16 bit wide (reminiscent to the old Win16 times).

If I'm not missing the point completely, SV_DECL_OBJARR does not use copy construction or object assignment when copying buffer content, so it will only work on objects having only basic data types as members, or members with this attribute (transitive), and no user-supplied copy constructor/assignment operator.

Both items have led to bugs which were somewhat hard to spot; the 16 bit limit easily leads to integer overflows, you then will overwrite the start of the array when you think you're writing to index 65570.

Therefore the bottom line is to use the STL container, since they are now ubiquitous available, provide real type safety and do not contain the drawbacks described above. But caveat emptor, the STL poses their own subtleties and gotcha's, reading something like “S. Meyers, Effective STL” is thus recommended.

There are also other legacy container classes that are better replaced with standard ones from the STL (at least in new code), for example class List and the associated DECLARE_LIST macro from <tools/list.hxx>.

Indication:

whenever you write new code, or extend code that had no Sv*Array before, use the appropriate std container (mostly std::vector<>, but maybe your data are constrained in such a way that e.g. std::map<> would fit better) instead of Sv*Array

Contraindication:

when maintaining code that already uses Sv*Array, it should be carefully inspected whether one of the problems described above

can occur

and is not dealt with already.

If this is the case, refactoring the code towards using the std containers should be considered. Otherwise, there is no need changing working code.

when maintaining code that uses SV_OBJARR, it is strongly recommended to refactor and use std::vector<> instead, since during array growth, SV_OBJARR uses raw memmove() to copy the existing objects, preventing proper calls to copy constructor/assignment operator

Examples:

TBD. Does somebody have any?

Don't fetch local data or objects from the heap, if they are not of user-defined size

During Win16 times, the stack segment was usually limited to 64k. Therefore, a common way to save on stack usage was to allocate on the free store even local method data. This is no longer necessary, and since C++ allows creation of automatic objects at any place in the code, not even the on-demand creation is an advantage. In contrary, the load and fragmentation this kludge generates for the heap manager leads to increased runtime and potential increased memory usage.

Thus, this idiom is deprecated. If you find places where it is still in use, you are encouraged to change them. Even if you have to deal with user-supplied data or array sizes, try to use std::vector<> or similar, which can also be used as an automatic variable, thus releasing you from the burden to manage the underlying memory by hand.

Indication:

- everywhere, except when size of data is user-defined

Contraindication:

- nowhere, except when size of data is user-defined (but check if you can get away with std::vector<> or similar, which also handles resource allocation for you)

Examples:

Use

{

String aStr;

}

instead of

{

std::auto_ptr< String > pStr ( new String() );

}

never use

{

String* pStr = new String();

...

delete pStr;

}

But beware, you must not use auto_ptr<> with an array:

{

std::auto_ptr< Point > pArray ( new Point[42] );

}

since auto_ptr<> does not call the proper delete[] destructor on your array (how should it? It does not know anything about the type you fed in).

Documentation

Comment your code for autodoc

You have to comment your code, both in header and in implementation files. Employ a style similar to JavaDoc, which can be parsed by OpenOffice.org's own documentation tool, autodoc. See here for a syntax documentation.

Comment your bugfix with #BUGID#

When fixing bugs, try to mark your fix with the bug ID of the task, if you have any. This will clearly identify the fix as such, even without cvs. Thus, if later on somebody frowns at that code, she can check back with your detailed description in the bug. Therefore, it is also advisable to comment your findings and the reason for the specific way to fix the bug in the bug description.

Indication:

- everywhere, especially for awkward or non-obvious fixes

Contraindication:

nowhere

Coding Guidelines for C++ in General

General Principles

First correct, second readable, optimize later

Summary

While coding, the first goal is correctness - letting the program do exactly what you want.

Second goal is readability. Another person (or yourself a year later), who has to fix a bug or continue work on this code, should easily understand what the code does.

Optimization comes only after the previous goals. Optimizing through choice of algorithm can be done earlier. But optimizing by "sophisticated" code should be postponed until a profiling makes clear where the critical parts are, which are worth optimizing.

Reason

If the code doesn't work, speed is useless. If the code is unreadable, after some time (at least in as large a project as StarOffice) it won't work. Also its more efficient to use a profiler to find the 20% of the program who do 80% of the work and optimize there, than to spend hours on optimizing functions that get called one time in a year per user.

Simple and defensive

Summary

Most of the times in coding, do simple things, where you understand exactly, what they do. If you are not sure, how a special algorithm or API-function works, but know a different way - maybe less optimized or less elegant - to do the task, consider to do it less elegant but simple, correct and understandable.

Reason

This is an essential part of writing bug-free code. Only things you know perfect and have a lot of experience with, can be bug-free.

Once and only once

Summary

Avoid duplicate code wherever possible. Make a function, a class or a template out of the common code and use that.

Reason

In duplicate code you have to fix every bug multiple times and change every new feature at multiple places.

Also possibly one day, one of the different code parts, that would need to change in duplicate code, will be forgotten.

Spend some time for meaningful and consistent names

Summary

Avoid names like "object" "thing" "part" and "reference", which do not say anything about the specific task of an object or type. Instead choose a name, which tells something essential about the task of the named thing.

Avoid shortcuts whenever possible. If a shortcut is necessary, put a comment to the declaration of the name, where the "complete" meaning is explained.

Reason

Meaningful naming is an essential part of readable code.

Consider the 80-20 rule

TBD.

Use "Patterns", but careful

TBD.

YAGNI: You aren't going to need it

TBD.

Class Design

One class - one responsibility

Summary

A class should have exactly one responsibility - no more, no less - and should perform this responsibility good and complete.

Reason

If classes are mixing different responsibilities, they produce unnecessary dependencies. Also they become more difficult to use and understand.

Negative Example

/** This class calculates the average of scientific measurements

and because its just at hand, it also stores the working times

of the scientist.

*/

class DoWork

{

public:

double CalculateAverage() const;

void AddValue(double i_measurement);

void StorePresenceTime(MyTime i_arrive, MyTime i_leave);

// [ ... ]

};

Positive Example

class Measurements

{

public:

double CalculateAverage() const;

void AddValue(double i_measurement);

// [ ... ]

};

class Scientist

{

public:

void StorePresenceTime(MyTime i_arrive, MyTime i_leave);

// [ ... ]

};

Make single argument constructors explicit

Summary

When there is a constructor with only one parameter, the explicit keyword. This also refers to constructors, where the last parameters are defaulted, so it could be used with one argument.

Reason

The explicit keyword avoids implicit typecasts, where you did not intend them. In complex programs this can lead to incorrect behavior. For instance, if writing a typo or forgetting to implement a function, that is called already - the compiler may look for a "best conversion" and find something, that was not intended to work, but does, because single-argument constructors are a candidate for implicit conversion.

Negative Example

class String

{

public:

// Creates the textual representation of i_number.

String(int i_number);

};

Positive Example

class String

{

public:

// Creates the textual representation of i_number.

explicit String(int i_number);

};

When you need a virtual or a protected destructor

Summary

Virtual destructors are needed exactly then, when: a derived class could be deleted by a pointer to a base of itself.

In that case both, the derived and the base class, have to be virtual and both destructors have to be virtual.

In some cases, it shall be impossible to delete a class via its base. Then make the base class destructor protected - it doesn't need to be virtual.

Reason

Deleting a class by a pointer to its base, which has a non virtual destructor, leaves the derived class' destructor uncalled. This causes resource leaks, and maybe crashes, too.

Negative Example

class Base

{

public:

~Base();

virtual Foo();

// [ ... ]

};

class Derived : public Base

{

public:

~Derived();

virtual Foo();

// [ ... ]

};

Positive Example

class Base1

{

public:

virtual ~Base1();

virtual Foo();

// [ ... ]

};

class Base2

{

public:

void DoSomethingInteresting();

protected:

~Base2();

// [ ... ]

};

class Derived : public Base1, public Base2

{

public:

virtual ~Derived();

virtual Foo();

// [ ... ]

};

void fx()

{

Derived * p1 = new Derived;

Base1 * p2 = p1;

delete p2; // correct: Calls virtually ~Derived()

p1 = new Derived;

Base2 * p3 = p1;

delete p3; // Incorrect, but dont worry, the compiler won't let this pass.

}

When to declare destructor, copy constructor and operator=()

Destructor

A class needs a destructor, if it has to free resources manually.

Prior to auto_ptr<> this was often the case with members on the heap. Nowadays, use auto_ptr<> or a similar class instead (see 2.12 "Distinct between owning pointers and referencing pointers").

Also classes with many data members should define a destructor, because the compiler might make it inline else. Which increases code size much, if the destructor is called from many places.

Copy constructor and operator=()

A class needs a copy constructor and operator=(), if if it doesn't have valid copy-semantics.

Classes have valid copy-semantics, if they consist only of the following kinds of data members:

non-pointer built in types (int, double, bool etc.),

referencing pointers (the pointed-to object is not owned by the present class),

classes with a public canonical copy constructor and operator=() defined,

classes, whose all members belong to one of the three cases above.

If a class doesn't fulfill these criteria, it needs to have copy constructor and operator=() declared. If you do not wish the class to be copied, declare them nevertheless, but as private.

In declaring copy constructor and operator=(), use the correct signatures:

ClassName(const ClassName& i_Other);

ClassName& operator=(const ClassName& i_Other);

In both functions it is important, that the parameter is const and that it is a reference!

The operator=() should return a non-const reference to the object it belongs to, because this is the behavior of the built in types (and the STL).

Test for self-assignment in operator=()

Failing to do so, can corrupt an object.

Initialize/assign always all data members

Summary

In every constructor and operator=() and also in functions like WriteMyselfToStream(stream & o_Destination) or ReadMyselfFromStream( stream & io_Source) initialize / assign always every data member. Even if it is not necessary or not intended to be assigned. In the latter cases, initialize it with the default or write the assignment in a comment line, with an explanation, why it is not assigned here.

Reason

Doing this consistently, it is very easy to check, if you have forgotten a member in these functions. This avoids uninitialized data.

Initialize data members in the initialization section

Summary

Use the initialization section of the constructor, i.e. between : and {, instead of the constructors function body.

Reason

Each member data is assigned a value in the initialization section anyway. So, assignments in the constructors function body do the work a second time.

Of course there is an exception: Consider if assigning in the body is necessary, when you use member data or functions to initialize other members.

Initialize data members in order of declaration

Summary

When initializing the data members of a class, write the initializer expressions exactly in the order, in which the members are declared.

Reason

The compiler initializes the members in this sequence anyway. If you write them in a different order, the reader of the code gets a wrong impression about what really happens.

The exact order of initialization is:

All virtual base classes (even if they are inherited via additional levels) - from left to right,

the direct base classes - from left to right,

the data members - from top to down.

Make all data members private

Summary

Make all data members of a class private.

( Some people consider plain data-aggregation-structures with no functions as useful. Those are not in the scope of this paragraph. )

Reason

As for public data: Encapsulation is widely known as a primary guarantor for bug-freeness and safety.

But why not make data protected? If you touch data members from a derived class, the responsibility for those data and for their consistency becomes shared - and so it gets unclear. If each class - even in a derivation-hierarchy - is completely and alone responsible for its own data members, this is the easiest way to ensure consistency and to find bugs, if they appear.

Use getters and setters with care

Summary

Some "getter"-functions are useful. But not every data member needs a corresponding getter.

Avoid "setters" in most cases.

Reason

Encapsulation is not about formally hiding data, but accessing them directly nevertheless, via getters and setters.

Instead the object should have a "behavior" - it should perform tasks. And the member functions should trigger performing those tasks. The functions may need parameters, but it should not be necessary for the user of the object to know, if a function parameter corresponds to some data within the class or are just calculated or accessed in any other way.

Negative Example

/* With this window class, you have no behavior,

but just a set of data.

Also, the implementation is difficult to change.

*/

class Window

{

public:

// [ ... ]

Get_Left() const;

Get_Top() const;

Get_Right() const;

Get_Down() const;

Set_Left() const;

Set_Top() const;

Set_Right() const;

Set_Down() const;

private:

int left;

int top;

int right;

int down;

};

Postive Example

/* This window class allows some actions on the window, without the need

to know member data.

*/

class Window

{

public:

// [ ... ]

std::pair<int,int> PositionOfTopLeftCorner() const;

std::pair<int,int> Size() const;

void	Move(int i_HorizontalDiff, int i_VerticalDiff);

void	MoveTo(int i_NewPosHorizontal, int i_NewPosVertical );

void	Resize(int i_Percent);

void	ResizeTo(int i_NewSizeHorizontal, int i_NewSizeVertical);

private:

// Different possibilities for implementing data:

// [ ... ]

};

Do not use virtual functions in constructors or destructors

Summary

Do not call virtual functions within constructors or destructors. There, they behave different then normally.

Reason

A virtual function can only be called on those levels of an object which already exist. Therefore in constructors or destructors, when you call a virtual function, not the function of the "real" object is called, but just the one on the inheritance level, where the current constructor or destructor belongs to.

Though sometimes this may be intended, most times it is not.

Consider the NVI (non virtual interface) idiom

TBD.

Distinguish between owning pointers and referencing pointers

TBD.

A destructor must not throw exceptions

TBD.

Class Inheritance

Do not violate the LSP (Liskov Substitution Principle)

Summary

The Liskov Substitution Principle (by Barbara Liskov) states, that everything you can do with a base class, must also be possible to do with all of its derived classes.

In other words: A derived class' interface must not have constraints, the base class interface has not.

Or more formally: Each implementation of a virtual overridden function in a derived class, must have no stronger preconditions and no weaker postconditions than the implementation (or promised interface) in its base class.

Reason

This is the fundamental of using virtual functions and abstract interfaces. It implies, that the user of a base class does not need to know, which derivations it has and what their attributes or constraints are. Instead the user of a function can be sure, the function will do, what she expects, whatever "real" class is performing the task.

Negative Example

/* The "classic" rectangle-square problem:

*/

class Rectangle

{

public:

Rectangle(int i_Height, int i_Width);

// [ ... ]

virtual int Area() const;

virtual void Resize(int i_NewWidth, int i_NewHeight);

private:

// [ ... ]

};

class Square : public Rectangle

{

public:

// [ ... ]

Square(int i_EdgeLength) : Rectangle(EdgeLength, EdgeLength) {}

virtual int Area() const;

virtual void Resize(int i_NewWidth, int i_NewHeight);

private:

// [ ... ]

};

void fx()

{

Rectangle * pRect = new Square(15);

int nArea = pRectArea(); // Still Ok, will be 225.

pRect->Resize(12,20); // Oops! - This is no longer a square.

// Because you can not resize a square like you can a

// rectangle, it violates the LSP, to derive square from rectangle.

}

Positive example

/* Now Resize() has different arguments for Rectangle and Square

and only common parts are derived from PlainGeometricObject.

Everything you can do with PlainGeometricObject, is also possible with

Rectangle and with Square.

*/

class PlainGeometricObject

{

public:

// [ ... ]

virtual int Area() const;

};

class Rectangle : public PlainGeometricObject

{

public:

Rectangle(int i_Height, int i_Width);

// [ ... ]

virtual int Area() const;

void	Resize(int i_NewWidth, int i_NewHeight );

private:

// [ ... ]

};

class Square : public PlainGeometricObject

{

public:

// [ ... ]

Square(int i_EdgeLength);

virtual int	Area() const;

void	Resize(int i_NewEdgeLength);

private:

// [ ... ]

};

Use public inheritance, else use composition

Summary

If you wish to inherit private or protected from another class, this often is an indication, you need the other class only for implementation. In that case its better to make the other class a member.

Reason

This has proved by experience to produce simpler and easier understandable code. Also, often inheritance in such cases would violate the LSP (3.1).

Make non-leaf classes abstract

Summary

Consider you have two concrete, instantiatable classes C1 and C2. Because there are similarities, you decide, C2 should inherit C1. In all such cases, find the common abstract interface - we may call it Cbase - of C1 and C2 and derive them both from Cbase. Make Cbase abstract. Do not derive one concrete class from another.

In other words: Make all non-leaf classes of an inheritance hierarchy abstract.

Reason

It is impossible to implement operator=() consistently, if one concrete (instantiatable) class inherits another one. (See Meyers "More Effective C++", Guideline 33).

If a class can be both, a base class and a leaf class, this can produce several problems and inconsistencies and the need for extra checks. It also violates the "one class - one task" principle.

If a member function is virtual, write the "virtual" also to all derived instances

Summary

A derived function is virtual, if the same function is declared virtual in the base class. Though it is not necessary to write virtual in front of the derived classes declaration of the function, it should be done nevertheless.

Reason

This is for readability. The reader of the code should see immediately, if a function is virtual or not.

Use inheritance only for is-a relationships

TBD.

Use composition for implementation relationships

TBD.

Function Design

One function - one task

Summary

A function should perform exactly one task - no more, no less - and should perform this task good and complete.

Reason

If a function performs several tasks, you get into problems, if a user of the function only needs one of their tasks performed. Either you have a second function which does similar things, which is ugly - or you use the same function and "redo" the unintended parts of its task, which is even uglier.

Small functions with one task each are easier to combine to perform exactly, what a client wants.

Make functions small

Summary

If you have significant sub-tasks, put them into a separate function.

If you have more than two levels of if-conditions or loops, put them into a different function.

If you need a comment for a paragraph of code within a function body, this indicates a chance, to make a separate function of it, and name this function after the comment, you intended to write.

Reason

This is a natural follow-up of One function - one task. Also, large functions are difficult to read and understand.

Use canonical signatures and behavior in operator overloading

Summary

When overloading operators, use exactly the signature, which mirrors the behavior of this operator on integers.

Always implement operator+() by means of operator+=(), the same for -, *, /, % etc.

Always implement the other logical comparison-operators by means of operator<() and, if this really increases performance, operator==().

Reason

These rules makes sure, overloaded operators behave exactly like the user expects. The last rule – implementing all comparison operators by operator<() – supports the STL. It allows classes to be valid citizens of STL-containers and algorithms, which only implement operator<(), but not the other 5 comparisons.

Be aware of the difference of pre- and postfix -increment and -decrement

Summary

Use the prefix-versions of increment and decrement whenever possible.

This is, how to declare increment and decrement-operators as member-functions:

MyClass &	operator++(); // prefix-operator++()

const MyClass operator++(int);	// postfix-operator++(),

// is distinguished by an unused int-parameter.

Reason

The prefix version does not produce a temporary, which the postfix version does. While this is irrelevant with integers or pointers, it can be inefficient, if an iterator is a class. See also Meyers "More Effective C++", Guideline 6.

Use conventions: Where you mirror STL-behavior, use the same function names and signatures

Wherever they appear, push_back() or empty() and other function names of the STL should have the behavior/meaning, they have within the STL. That is, what the user of the code expects them to do.

Indicate in-, out-, inout- and pass_ownership-parameters

Summary

Use a consistent way to make absolutely clear, if a parameter is an out- or inout-parameter. Also make it obvious, if a parameter passes ownership of a heap-object.

There are two possible ways:

Start each parameter's name with one of the prefixes i_, o_, io_ or pass_ . This has also the advantage to make a difference between function parameters and other local variables.

Or start only the non-in-parameters with obvious prefixes, like out_, inout_, pass_ .

Reason

This is for readability. The reader of the code should see immediately, what sort a parameter is of.

Make all in-parameters called by value or by const-reference/pointer

Summary

Pass all in-parameters by value or by const-reference or const-pointer.

Reason

To make sure in-parameters are not only documented as "in-", but cannot be misused by error.

Use conventions: Put out- and inout-parameters first

TBD.

Use pointers where 0 is an allowed value, references where it is not

TBD.

Use return value optimization

TBD.

Consider return by value instead of return by const reference

TBD.

Const Correctness

Make reference in-parameters in functions always const

TBD.

Make member functions const where possible

TBD.

Use mutable

TBD

Be aware of the difference between "const xy *" and "xy * const"

TBD.

Statement level idioms

Use the new C++ casts instead of C-style casts

Summary

Do not use C-style-casts: type(expression) or (type)(expression).

Instead become acquainted with and use only the C++-casts: static_cast<>, const_cast<>, reinterpret_cast<>, dynamic_cast<>.

Reason

The code becomes better readable. Also the meaning of the C++-casts is more exact and some subtle errors are easier detected by the compiler. And it is easier to find casts during debugging or with grep. See also Meyers "More Effective C++", Guideline 2.

How a for-loop should look like

Summary

A for-loop with iterators should look like this:

MyContainer::iterator itEnd = aContainer.end();

for ( MyContainer::iterator it = aContainer.begin(); it != itEnd; ++it )

{ /* do whatever */ }

Also consider, if it is possible to use T::const_iterator instead of T::iterator.

Reason

The end() function can be complex, therefore it should not be called for every iteration.

Increment the iterator by prefix-++, not by the postfix-version. This avoids unnecessary temporaries.

Ranges should include lower bound, exclude upper bound

Summary

Give and use all ranges with lower-bound <= values < upper-bound.

Reason

This is done this way everywhere in the realm of C/C++. Doing different will lead to misunderstanding of the code.

Keep local entities local

Sometimes, entities (variables, functions) that should be local to a translation unit are mistakenly declared to have global linkage (and thus appear as global symbols in the resulting shared libraries). Randomly picked examples of definitions/declarations appearing at file scope are

USHORT nBestEntry = 0;

leading to global symbol nBestEntry (representing a global variable),

sal_Int16 nConvertSlots[] = { ... };

leading to global symbol nConvertSolts (representing a global variable), and

int my_inflate_codes(...);

leading to global symbol my_inflate_codes (representing a global function). Lets assume that, in all three cases, the intent was to create entities with internal linkage (i.e., entities that can only be accessed from within the same source file) instead of entities with external linkage (i.e., entities that can be accessed from within any source file).

Mistakenly using entities with external instead of internal linkage can cause various troubles:

1 The code generated by the compiler to access the entity (in the above examples, accessing the variables nBestEntry or nConvertSlots, or calling the function my_inflate_codes) is probably bigger/slower than necessary.

2 At load time (variables) or runtime (functions), expensive symbolic relocations are caused.

3 The symbol can conflict with other global symbols, leading to link errors or erroneous runtime behavior.

4 On a related note, if the entity is a constant rather than a variable, saying so explicitly (with the const keyword) could reduce the size of the data segment (of which there is one per processes) and instead enlarge the text segment (shared by all processes), reducing overall memory consumption.

To prevent these troubles, use the following constructs instead of the above examples:

A If the entity is a constant, use const:

USHORT const nBestEntry = 0;

sal_Int16 const nConvertSlots[] = { ... };

(or put the const in front of the type, which is identical). C++ has the rule that “an object or reference that is explicitly declared const and neither explicitly declared extern nor previously declared to have external linkage” has internal linkage (C++ Standard, 3.5/3). This prevents troubles 1–4. If you want to explicitly express in the program code that the constant's name is only valid within the current .cxx file (instead of relying on 3.5/3), you can additionally use constructs B or C (but note that doing so might re-introduce troubles 1 and 2).

B Use static:

static USHORT nBestEntry = 0;

static sal_Int16 nConverSlots[] = { ... };

static int my_inflate_codes(...);

C++ guarantees that “an object, reference, function of function template that is explicitly declared static” has internal linkage (C++ Standard, 3.5/3). This prevents troubles 1–3. But note that “the use of the static keyword is deprecated when declaring objects in a namespace scope [...]; the unnamed-namespace provides a superior alternative" (C++ Standard, 7.3.1.1/2); see also http://www.cuj.com/experts/2006/hyslop.htm. This means you might want to consider construct C instead (but note that doing so might re-introduce troubles 1 and 2).

C Use an unnamed namespace:

namespace { USHORT nBestEntry = 0; }

namespace { sal_Int16 nConvertSlots[] = { ... }; }

namespace { int my_inflate_codes(...); }

The entity will have external linkage, but the symbol will be mangled in such a way that it cannot conflict with other global symbols. This means that trouble 3 will be cured, but also means that most probably troubles 1 and 2 are not cured (unless you have a pretty smart compiler/linker). To cure at least trouble 2, there are mechanisms available, depending on how a given OOo module restricts exports of shared objects:

C1 In case the module uses the old, Windows-only filter file mechanism (DEFnDEPN in makefile.mk):

The MSVC compiler mangles symbols from unnamed namespaces so that they contain the complete path of the source file. In a filter file (used during linking of a shared library to filter out global symbols that match certain patterns, reducing them to local symbols), this fact can be exploited by adding the pattern modulename\ (where modulename is the name of the OOo module, which is guaranteed to be part of the complete path of any source file within that module). For example, the filter file svtools/util/svl.flt contains the line

svtools\

This has the drawback that it currently only works on Windows (since the whole filter file mechanism is only implemented on Windows; other platforms have similar conventions of mangling symbols from unnamed namespaces, leading to similar patterns that could be filtered out).

C2 In case the module uses the new map file mechanism (SHLnVERSIONMAP in

makefile.mk):

Those map files (with a syntax dictated by the Solaris and Linux linkers) typically have a section

local:

*;

which causes all global symbols that are not explicitly mentioned elsewhere within the map file to be reduced to local ones. So, symbols from unnamed namespaces will automatically get reduced to local in this case.

So, which is the preferred mechanism to use (A, B, C, C1, C2)? Good question. Probably, C2 (together with A, if applicable) is best, followed by B (together with A, if applicable).

Avoid reserved names

TBD.

Avoid duplicate names

TBD.

Optimize by algorithms and by profiler instead of "tricks"

TBD.

Use inlining only where obvious

TBD.

Use standard (STL) containers and algorithms

TBD.

Use auto_ptr<> / Dyn<> for free / member- objects on the heap

TBD.

Declarations in for-loops

TBD.

Catch exceptions by reference

TBD.

Do not use "magic numbers"

TBD.

Initialize all variables at declaration

TBD.

Beware of signed/unsigned mismatches

TBD.

Consider when to use fixed-size data types versus builtin types

TBD.

Avoiding Macros

TBD.

Use "const" instead of #define

TBD.

Use inline functions instead of macros

TBD.

Use templates instead of macros

TBD.

Dependency Management

Program against interfaces

TBD.

Typedef types, if they might change

TBD.

Avoid switch statements

TBD.

Consider the pimpl idiom

TBD.

Put private inline functions in the .cxx file

TBD.

Header Inclusion

Prefer forward declarations to header file inclusion

The order of header file inclusion should not be important

TBD.

Use include guards

TBD.

Documentation and Comments

Use autodoc-style documentation in header files

TBD.

For each class, document its responsibility

TBD.

For each function, document its task

TBD.

Use C++-style comments

TBD.

Document every unusual behavior

TBD.

Use function names instead of comments where possible

TBD.

Comment tricks, hacks, unfinished code and complex algorithms

TBD.

Random Bag of Coding issues

Memory Visibility between Threads

C++ itself does not address any multithreading related issues, but each multithreading package (e.g. pthreads) should specify things similar to what is outlined here: If one thread changes any memory locations, do not assume that another thread can see all these changes, unless those threads use a synchronization primitive (e.g., a mutex in the pthread case) to synchronize. This applies even if reading or writing the memory location is typically an atomic operation (e.g., for boolean variables).

That is, the code

int i = 0;

bool b = false;

// Thread 1:

i = 10;

b = true;

// Thread 2:

if (b) cout << i;

has a problem. You might think that, depending on whether the code in thread 1 or thread 2 runs first, either nothing is written out, or the value 10 is written out, but that need not be the case. If the code in thread 2 runs before the code in thread 1, thread 2 will see that b is false and skips writing i. But if the code in thread 2 runs after the code in thread 1, any of three things can happen: Thread 2 has not yet noticed that thread 1 updated b, so thread 2 still sees that b is false and skips writing out i. Or thread 2 has already noticed that thread 1 updated b, has also already noticed that thread 1 updated i, and writes out the correct value of 10. Or thread 2 has already noticed that thread 1 updated b, but has not yet noticed that thread 1 updated i, and writes out the wrong value 0. The third alternative (thread 2 sees some, but not all, changes made by thread 1) is what causes this code to fail.

Therefore, the above code should be changed to

osl::Mutex m;

int i = 0;

bool b = false;

// Thread 1:

{

osl::MutexGuard g(m);

i = 10;

b = true;

}

// Thread 2:

{

osl::MutexGuard g(m);

if (b) cout << i;

}

or something similar. This code either writes out nothing, or writes out the correct value of 10, but it never writes out the wrong value of 0.

Note that this is exactly the reason why the double-checked locking pattern is broken, and should not be used in portable C or C++ code. If you want to use a fixed version of double-checked locking, there are a C macro (OSL_DOUBLE_CHECKED_LOCKING_MEMORY_BARRIER in <osl/doublecheckedlocking.h>) and a C++ template (rtl_Instance in <rtl/instance.hxx>) that allow you to do so—see the documentation in the respective header files. Also, the forthcoming ODK Developer Manual will contain a section on double-checked locking.

The book Programming with POSIX® Threads by David R. Butenhof (Addison-Wesley, 1997) details the restrictions relevant to pthreads regarding memory visibility between threads. There is probably a similar description for the Win32 API.

Integral Types

When using integral types, either the standard ones like int, unsigned int and std::size_t, or the ones from sal like sal_Int32 and sal_uInt32, note the following points:

There are only some guarantees about the minimal range of numbers supported by each type (e.g. unsigned int will support the range from 0 to 65535, and sal_Int32 will support the range from -2147483648 to 2147483647). There is nothing said about the actual range supported (which may differ among platforms, but will always include the guaranteed minimal range).

Unsigned integral types use arithmetic modulo 2n, whereas overflow for signed integral types incurs undefined behavior. That is, the class

class Counter1

{

public:

Counter1(): c_(0) {}

sal_uInt32 next() { return c_++; }

private:

sal_uInt32 c_;

}

only has the problem that it wraps around after 232 or more invocations of Counter1::next (whether that really is a problem or not), whereas the class

class Counter2

{

public:

Counter2(): c_(0) {}

sal_Int32 next() { return c_++; }

private:

sal_Int32 c_;

}

potentially crashes the machine after enough calls to Counter2::next.

Using the Observer Pattern

We often use the observer pattern in our APIs and implementations, but using it has certain implications in a multi-threaded scenario. Consider the following simple incarnation of the observer pattern. It is written in Java, but what is discussed here equally holds for C++ or about any other imperative language; Java just seemed to result in the easiest example code.

class Subject {

public synchronized int getValue() { return value; }

public void setValue(int val) {

int old;

Observer obs;

synchronized (this) {

old = value;

value = val;

obs = observer;

}

if (obs != null) {

obs->notify(???);

}

}

public synchronized void setObserver(Observer obs) {

observer = obs;

}

private int value = 0;

private Observer observer = null;

}

class Observer {

public void startObserving(Subject sub) {

sub.setObserver(this);

}

public void notify(???) {

// do something, e.g., display the Subject's current

// value to the user:

System.out.println("Value: " + ...);

}

}

Class Subject holds a value that can be read and written in a thread-safe way using getValue() and setValue(). Whenever the value is written to, the Observer is notified (here, there is only one Observer to keep the example simple, but normally an arbitrary number of Observers can be registered with one Subject).

As an aside, note that getValue() needs to be synchronized even in this simple case where the value to be read consists of only a single int, to avoid one thread reading a stale value when another thread already has written a new value using setValue().

And note that, as is usual, calling notify on the Observer object is done outside of the synchronized block in setValue(), to avoid deadlocks.

The question now is, what should be the arguments to notify—the new value, both the old and the new value, or none?

Often, you see the pattern used so that notify() carries the new value from Subject to Observer, i.e., with setValue() being

public void setValue(int val) {

Observer obs;

synchronized (this) {

value = val;

obs = observer;

}

if (obs != null) {

obs->notify(val);

}

}

A reason to follow this strategy might be that the code in Observer's notify() needs the new value anyway, so it is easier to pass it as an argument to notify(), rather than requiring an extra call to getValue(). Our simple notify() would then look something like

public void notify(int value) {

System.out.println("Value: " + value);

}

In a multi-threaded scenario, this strategy is highly problematic, as it can lead to the Observer reporting wrong values. Imagine that the Subject's value is initially 0, and that one thread calls setValue(1) while another thread calls setValue(2) at about the same time. Updating the internal value of Subject is synchronized, so one thread will write its value first, and then the other will overwrite that temporary value with the final one. Which thread will go first is unpredictable, but lets assume that the first thread goes first and sets the value to 1. After that, the second thread sets the value to 2, the final value. Each thread sends a notification to the Observer, containing the value that thread set. But since sending the notifications is not synchronized (and it better not be, to avoid deadlocks in general), it is unpredictable whether the Observer will print

Value: 1

Value: 2

or

Value: 2

Value: 1

on the screen. If we want the last line printed by the Observer to always reflect the Subject's current value, then the above code clearly failed in the scenario just described: the Subject's current value is 2, but the last line printed by the Observer contains 1.

This shows why sending the new value along with the notification is not a good idea. The Observer cannot rely on the value sent indeed being the Subject's current value, so it has to call getValue(), anyway.

You can try to be extra clever, and send both the old and the new value when calling notify():

public void setValue(int val) {

int old;

Observer obs;

synchronized (this) {

old = value;

value = val;

obs = observer;

}

if (obs != null) {

obs->notify(old, val);

}

}

The cleverness is that you can now code the Observer so that it can detect whether it really is sent the current value, or some stale one already overwritten by another call to setValue(). Only in the case when it is sent a stale value does it need to call getValue() to get the real value—or so you might think:

class Observer {

public void startObserving(Subject sub) {

subject = sub;

subject.setObserver(this);

cached = subject.getValue();

}

public synchronized void notify(int oldVal, int newVal) {

if (oldVal != cached) {

newVal = subject.getValue();

}

cached = newVal;

System.out.println("Value: " + newVal);

}

private Subject subject;

private int cached;

}

The idea is simple: You know the Subject's initial value (lets call it A), and you observe a number of (more-or-less concurrent) changes that ultimately change the Subject's value to B (it can take on a number of other temporary values in between). Regardless in which order the notifications reach you, if the old value given in a notification always equals your cached value (which you then update with the new value from the notification), then you can be sure that in the end, after all the notifications have reached you, in whatever order, you always know the correct final value B. (Sketch of proof: Consider the changes to be dominoes, with the old value on the left and the new value on the right. The actual series of changes will result in a chain of dominoes starting with A, ending with B, and with all junctions between two dominoes showing matching symbols. So each symbol will occur an even number of times on the dominoes, except for A and B, which appear an odd number of times. However you reorder the dominoes into a valid chain starting with A, it will always have to end in B, since there is this extra B that can only go at the end.)

The problem is that you do not know the Subject's initial value (i.e., you do not know what A is). After calling setObserver(), while you call getValue() to determine the Subject's initial value, already notifications can come in about modifications. Assume the Subject's initial value is A, it is then first changed to B by one thread, and then back to A by another. It may well be that by the time you call getValue(), Subject has already changed from A to B, so that you'll cache B as Subject's initial value. Then, by chance, you first get the second thread's notification that the value has changed from B to A (which matches your cached value B), and only then you get the first thread's notification that the value has changed from A to B (which again matches your cached value, which is now A). So you happily think that the Subject's value is now B, while in fact it is A. (By the way, there are even more multi-threading issues with the above code for Observer.)

Having seen that it is neither a good idea to send the new value as part of the notification, nor to send both the old and the new value, it should be clear that the best way is to send none:

public void setValue(int val) {

Observer obs;

synchronized (this) {

value = val;

obs = observer;

}

if (obs != null) {

obs->notify();

}

}

That way, the Observer always has to fetch the current value via getValue():

class Observer {

public void startObserving(Subject sub) {

subject = sub;

subject.setObserver(this);

}

public void notify() {

System.out.println("Value: " + subject.getValue());

}

private Subject subject;

}

Note that it is now possible that the Observer misses on some of the temporary, intermediate values when multiple threads change the Subject's value concurrently. Coming back to the earlier example, the Observer's output might now look like

Value: 1

Value: 2

or

Value: 2

Value: 2

but the last line should now always be faithful with the Subject's true value, or shouldn't it?

Unfortunately, the answer is still “no:” Since subject.getValue() and the call to System.out.println are not done in one atomic step in Observer.notify, multiple invocations of Observer.notify can overlap to produce

Value: 2

Value: 1

The apparantly easiest fix would be to make Observer.notify synchronized; but that would cause Observer to make a call on Subject while it holds a lock, and could thus lead to deadlock if no precautions are taken (i.e., the behaviour of Subject.getValue would need to be constrained in such a way that no deadlocks can occur when it is called in the above fashion; especially for UNO APIs, it seems to be pretty tough to formulate such a constraint in a language-neutral, UNO way).

Another solution is to implicitly synchronize the execution of all calls to Observer.notify, by creating a dedicated thread that receives requests through a queue: the thread repeatedly waits for the queue to become nonempty, takes the first element off the queue, does all actions that were originally done within Observer.notify (e.g., executes System.out.println("Value: " + subject.getValue());), and then waits again for the queue to become nonempty. Observer.notify would simply place an element at the end of the queue and return. In special circumstances, this solution is already used (e.g., in the form of the event loop of GUI frameworks), but for general purposes it seems to be too heavyweight to introduce an additional thread for each Observer.

Better solutions to the general problem are still needed.

It should be clear that only sending actual old or new values with notify() is bad. It can well make sense to send some structural information (e.g., if Subject represents an aggregation of n items, that only item k has changed) or the identity of Subject (if Observer listens on multiple Subjects and has to differentiate among them). The criterion whether to send any data in the notification should be whether this data would be invalidated by another change to the Subject or not.

Also, section 3.5.2 of Concurrent Programming in Java contains more on using the observer pattern in a multi-threaded scenario.

Observation 1: If the Observer is only interested in the Subject's current value (as in our example here), another approach can be to send the new value in notify(), but augmented with some sort of time stamp, so that the Observer can detect and correctly handle out-of-order notifications. This has the performance advantage of requiring only a single call from Subject to Observer (notify()), instead of a call in each direction (notify() plus getValue()). For APIs that are intended to be used remotely this added complexity (in both Subject and Observer) can well be worth it. But remember that it is generally best to stick with simple solutions unless there is clear evidence that performance will suffer.

Observation 2: If the Observer somehow does know the Subject's initial value, yet another approach can be to send differences instead of old and new values in notify(): The Observer can apply all the differences to the initial value, and, regardless of the order of notifications, will always end with the same value as the Subject. Note that this only works if the operation of applying differences is commutative, and that this is often not the case, even though it might seem so (e.g., in C and C++, addition over signed int is not commutative, while addition over unsigned int is.)

And, finally, if you use UNO to define the Subject and Object interfaces, the consequences of making notify() an asynchronous one-way call or an ordinary synchronous call are yet another story (still to be added to this document...).

Optimization

As a general rule, only optimize your code when the tradeoffs (increased coupling of software modules, increased complexity of source code) are low, or when there is proven evidence that optimization is needed (i.e., after profiling).

It is a bit hard to give an example of when optimizations are in place here (because even the simplest optimization typically increases the complexity of the code somewhat), but I will try anyway. Consider a function that gets two arrays of the same length as input, and has to determine whether those arrays contain equal elements. The elements need not be in the same order in the two arrays, but they will typically be (if the two arrays indeed contain only equal elements, which will also typically be the case). The simple version

bool match1(int const * p1, int const * p2, std::size_t len)

{

for (int i = 0; i < len; ++i)

{

bool found = false;

for (int j = 0; j < len; ++j)

if (p1[i] == p2[j])

{

found = true;

break;

}

if (!found)

return false;

}

return true;

}

has a runtime of O(n2), whereas the slightly optimized version

bool match2(int const * p1, int const * p2, std::size_t len)

{

for (int i = 0; i < len; ++i)

{

bool found = false;

for (int j = i;;)

{

if (p1[i] == p2[j])

{

found = true;

break;

}

j = (j + 1) % len;

if (j == i)

break;

}

if (!found)

return false;

}

return true;

}

has a runtime of only O(n) in the typical case (when the two arrays indeed contain equal elements, and the equal elements appear at the same position in the two arrays).

Aesthetics

There are probably no two programmers that agree in detail how to structure a source file, but it is nevertheless important to produce clean and readable code. Regardless of whether you write a new source file from scratch or modify an existing one, always try at least to use uniform naming and spacing conventions within a single file.

Refactoring

The book “Refactoring: Improving the Design of Existing Code” by Martin Fowler, Kent Beck, John Brant, William Opdyke and Don Roberts (Addison-Wesley, 1999) should encourage you to consider your code not as something fixed forever, but rather as something that can be modeled as you go along modifying existing code and writing new code. The bulk of that book is centered around Java, but a C++ programmer should benefit, too (if only in finding confidence that refactoring is a good thing that indeed works).

The Proper Usage of Selected Often Used APIs

TBD: Best practices using sal, rtl, osl and filesystem interfaces.

Best Practices Using “sal” Interfaces.

Avoid using sal_Bool as far as possible.

Using sal_Bool may lead to subtle errors. Consider the following example:

// !!! the following example is buggy !!!

const int A_CONST_VALUE = 0x100;

sal_Bool FuncReturnsAlwaysFalse(int aValue)

{

return (aValue & A_CONST_VALUE);

}

The function in the example above always returns 0 which means false.

Why? sal_Bool is only a typedef for unsigned char - 8 bit data type; see <sal/types.h>. Suppose aValue is 0x100 then the (0x100 & A_CONST_VALUE) == 0x100 but the compiler casts the result to unsigned char alias sal_Bool which is the return value of this function. Because 0x100 is greater that 8 bit the result is always 0x00 (the lower part of 0x100).

Conclusion: When programming in C++ use the C++ bool type. When programming in C be aware of the problem.

The correct sample for the false code above would be:

const int A_CONST_VALUE = 0x100;

sal_Bool FuncCorrect(int aValue)

{

return ((aValue & A_CONST_VALUE) > 0);

}

For further reasons why you should use the C++ bool type see “Herb Sutter: Exceptional C++”, page 187.

Use sal throw clause macros when you have to declare exception specifications.

When using an exception specification in a function declaration then use the sal throw clause macros. These macros have been introduced to reduce code size by balancing out compiler bugs. The macros are defined in <sal/types.h>.

These macros are ONLY for function declarations, use common C++ throw statement for throwing exceptions, e.g.

throw RuntimeException();

SAL_THROW() should be used for all C++ functions, e.g. SAL_THROW( () )

SAL_THROW_EXTERN_C() should be used for all C functions. Attention: there are good reasons not to use an exception specification see:

Herb Sutter: Exceptional C++, page 54

http://www.gotw.ca/gotw/082.htm

http://www.boost.org/more/lib_guide.htm#Exception-specification

When you have to define a main function use the macros from <sal/main.h>.

When you need to define an own main function use the macro SAL_IMPLEMENT_MAIN defined in <sal/main.h>. This macro respects the fact that you need different main functions under Windows depending on your target application (Console/Graphical User Interface (GUI)).

Example:

#include <sal/main.h>

SAL_IMPLEMENT_MAIN()

{

DoSomething();

return 0;

}

Avoid unnecessary SAL_CALL calling convention (Windows).

Under Windows you can choose among different calling conventions when declaring a functions. There are e.g. the Standard (__stdcall) and C (__cdecl) calling convention. The compiler creates different code depending on the calling convention. Under Unix there is only one calling convention - the C calling convention.

The calling convention SAL_CALL which is an alias for __cdecl must be used for UNO interfaces and other external interfaces.

For internally used functions etc. you don't have to and should not use SAL_CALL.

If you don't specify a calling convention the default calling convention will be used which is __stdcall in our build environment.

Reason: Under Windows SAL_CALL alias __cdecl creates larger and slightly slower code.

For further information see:

Microsoft Developers Network Library (MSDN): Calling convention Topics

Best Practices Using “osl” Interfaces

Use osl_getThreadTextEncoding when you want to convert strings from ANSI to UNICODE and vice versa.

When you want to convert strings from ANSI to UNICODE and vice versa you have to know which text encoding to use for the conversion. Some operating systems do allow to change the current used text encoding thread-local so there is no process-global text encoding. If you want to use the current, thread-local text encoding for your string conversion use osl_getThreadTextEncoding(), declared in <osl/thread.h>.

Use osl_incrementInterlockedCount, osl_decrementInterlockedCount in order to change the value of an integer thread-safe.

Instead of using a mutex or another synchronization object for manipulating the value of an integer variable in a thread-safe manner use osl_incrementInterlockedCount() and osl_decrementInterlockedCount(). Using these functions is the cheapest way to change the value of an integer variable in a thread-safe manner. Both functions are declared in <osl/interlck.h>.

When you need raw memory consider allocating it on the Stack instead of on the Heap.

This is no promotion for using raw memory but sometimes even this is necessary. So when it's unavoidably to use raw memory consider allocating it on the stack. The days are over where the stack of a thread was a very limited resource. Today the stack of a thread is usually 1 MB of memory and more. So if you need a temporary buffer of a certain size and you can be sure that it doesn't exceed the limits of the stack than allocate it on the stack using alloca() or something similar.

Reason: Allocating memory on the stack is much much faster than allocating memory on the heap.

The heap will be less fragmented due to allocation of small memory chunks.

The allocated memory doesn't have to be deleted (!) instead it will automatically be deleted when the function etc. exits – so it's even exception safe.

For further information see:

Documentation of your C-Library.

Be aware of the different load parameter for osl_loadModule.

The function osl_loadModule(), declared in <osl/module.h> supports different load parameter that effect the loading of modules under Unix. Use the parameter SAL_LOAD_MODULE_GLOBAL in order to avoid symbol ambiguities. Using this parameter effects everyone within the current process. That means that if you load a module containing a function Foo everyone within the current process will use this function Foo after successfully loading the module.

Use the osl Debug Macros for debug output.

osl offers different macros useful for debug output and assertions. These macros are defined in <osl/diagnose.h>. In order to unify the usage of debug macros everyone developing office code should use these macros. osl assertions may further be switched off even in debug build by setting the environment variable DISABLE_SAL_DBGBOX=t.

Be aware that the macro OSL_TRACE has currently no effect under Windows.

Don't use the the osl Profile class.

This class is deprecated. Use the Config class from the tools module instead!

Best Practices Using “osl” File System Interface

TBD.

Typical Problems within Office Coding

Byte Strings versus Unicode Strings

“Byte string” refers to the types char *, ByteString and rtl::OString.

“Unicode string” refers to String and rtl::OUString.

Within the Office code, there still happen to be places, where byte strings are used, but unicode strings should be used. To avoid this, make sure to use unicode strings in any case where you are in doubt, and to use byte strings only in the following cases:

If you are absolutely sure, the byte string does contain only ASCII characters.

Direct user input on the vcl/sal layer on platforms which support only that kind of strings. That does not apply for the application code, because they will see those strings transformed to unicode already.

External APIs that are based on byte strings. Here of course you have to match the interface.
Usually you should immediately convert received byte strings and you also should convert your own unicode string to a byte string only immediately before the call of the external API.
However, if you don't know the encoding of the byte string you exchange with an external byte string based API, it is better to keep the string in its original type, even internally. Though this will be merely a "heap of bytes" but a string. You shouldn't interpret it's content in any way!

Ensuring a Relocatable Office Installation

Some rules have to be obeyed In order to keep the OpenOffice.org installation directories relocatable. These rules as well as coding guidelines are outlined in http://tools.openoffice.org/policies/relocatable_office_installation.sxw in section “Rules for Developers” and “Helpers for Developers”.

Java Coding Guidelines

TBD.

Further Reading

C++ knowledge

“C++: Effective C++: 50 Specific Ways to Improve Your Programs and Design” by Scott Meyers (Addison-Wesley, 1998);

“More Effective C++: 35 New Ways to Improve Your Programs and Designs” by Scott Meyers (Addison-Wesley, 1996);

“Exceptional C++: 47 Engineering Puzzles, Programming Problems, and Solutions” by Herb Sutter (Addison-Wesley, 2000);

“More Exceptional C++: 40 New Engineering Puzzles, Programming Problems, and Solutions” by Herb Sutter (Addison-Wesley, 2002);

“Modern C++ Design: Generic Programming and Design Patterns Applied” by Andrei Alexandrescu (Addison-Wesley, 2001).

Libraries, STL:

http://www.sgi.com/tech/stl/ "Standard Template Library Programmer's Guide";

“The C++ Standard Library: A Tutorial and Reference” by Nicolai M. Josuttis (Addison-Wesley, 1999);

“Effective STL: 50 Specific Ways to Improve Your Use of the Standard Template Library” by Scott Meyers (Addison-Wesley, 2001);

“Boost”: http://www.boost.org "Boost C++ Libraries";

Patterns:

“Design Patterns: Elements of Reusable Object-Oriented Software” by Erich Gamma, Richard Helm, Ralph Johnson and John M. Vlissides (Addison-Wesley, 1995);

“Pattern-Oriented Software Architecture, Volume 1: A System of Patterns” by Frank Buschmann, Regine Meunier, Hans Rohnert, Peter Sommerlad and Michael Stal (Wiley, 1996);

“Pattern-Oriented Software Architecture, Volume 2: Patterns for Concurrent and Networked Objects” by Douglas C. Schmidt et al. (Wiley, 2000);

Profession:

“The Pragmatic Programmer: From Journeyman to Master” by Andrew Hunt and David Thomas (Addison-Wesley, 2000);

“Extreme Programming Explained: Embrace Change” by Kent Beck (Addison-Wesley, 2000);

“The Mythical Man-Month: Essays on Software Engineering, Anniversary Edition” by Frederick P. Brooks Jr. (Addison-Wesley, 1995).

Erich Gamma, Richard Helm, Ralph Johnson, and John Vlissides. Design Patterns: Elements of Reusable Object-Oriented Software. Addison-Wesley, 1995.

Doug Lea. Concurrent Programming in Java™, Second

Edition: Design Principles and Patterns. Addison-Wesley , 2000.

56

Jens-Heiner Rechtien

2002-06-18T16:01:22

Jens-Heiner Rechtien