C++ Errors to look out for

Programming errors and problems in C++ exercises, assignments and projects

Notes

This documents explains the comments I have written directly to the sources of written assignments and projects as a form of feedback. Please ask for further explanations if you have not understood a comment I have written.

Include guard missing

C++ header files should generally be protected from multiple inclusion using an include guard. Consider the header file my_header.hpp containing the definition of the class my_class:

// my_header.hpp (BAD IDEA)
class my_class {
  // ...
};

If my_header.hpp is included more than once, which can happen quite naturally, the compiler sees an illegal redefinition of the class my_class.

To eliminate the redefinition, the header file should implement an include guard using the C preprocessor:

// my_header.hpp (correct)

#ifndef MY_HEADER_HPP_INCLUDE_GUARD
#define MY_HEADER_HPP_INCLUDE_GUARD

class my_class {
  // ... };
#endif

The names of include guards should be unique.

Unfortunately, the C preprocessor does not have a built-in scoping mechanism. It is therefore important to try and make include guards unique. Otherwise two or more headers might have the same include guard. If your code happens to need definitions in both headers, you are going to be in trouble as you can't just include both of the headers (the include guard of the first header will prevent the processing of the second header). Direct translation of the name of a header to a macro isn't generally sufficient to ensure uniqueness. More robust techniques include a) adding your initials and a date to the include guard b) using a 128-bit GUID (globally unique identifier).

Implementation hiding does not mean sources must be deleted.

Implementation hiding doesn't mean that the original source code would be deleted after it has been compiled. Implementation hiding means that the interface and implementation of a class (or other program component) are separated in such a way that a client of the component is not depended upon the implementation.

Build-script (or Makefile) does not compile and link the whole project.

There should generally be just a single build-script that needs to be executed to build (compile and link) the whole project. If the project needs to invoke the compiler multiples times, it can be done in a single script.

This header should not be included here.

This comment is used for two related purposes. One purpose is to point out that a required #include statement is in the wrong place (wrong header / implementation file). Another purpose is to point out that the header should not be included at all in a particular source file (header or implementation).

As a rule, a source file (header or implementation file) should only include headers that are directly needed by declarations and definitions in that file. The C++ #include mechanism is basically a low level file inclusion mechanism. In a large project, unnecessarily included headers often account for the majority of compile time. Unless unnecessary includes are carefully eliminated right from the start, it can lead to severe compile-time problems once the project grows larger.

This header is not self-contained.

While it is wrong to include more than necessary, it is also wrong not to include enough. A header file should be a self-contained unit. In other words, a translation unit consisting of just a single header and an empty main -function should compile without errors:

#include <header_to_be_tested.hpp>
int main() {}

If the above program does not compile with a particular header, then that header is not self-contained.

More specific headers should generally be included first.

Quickly, what rule gives an almost zero effort technique for ensuring that headers are self-contained?

Whitespaceshouldbeusedtoimprovereadability.

Itisgenerallybettertoseparatelexicalelementsofprogramcodebywhitespace, because it considerably improves readability.

.h-suffix should be reserved for C. Use .hpp / .h++ / .hxx / .hh for C++ headers.

In some programming environments, files with the suffix .h are considered C and not C++ source files. Generally, whether a file suffixed .h is a C or C++ header is somewhat ambiquous. The same problem does not exist with headers with any of the suffices .hpp, .h++, .hxx or .hh, each of which has a commonly used implementation suffix counterpart .cpp, .c++, .cxx or .cc. Therefore it is recommended to reserve the .h suffix to C (and not C++) headers.

This file has an odd suffix. Prefer commonly used suffices.

For implementation files, use one of the following suffices: .cpp, .c++, .cxx, .cc. For header files, use one of the following suffices: .hpp, .h++, .hxx, .hh. These are the commonly used suffices for C++ implementation and header files. Avoid inventing new suffices.

This header is unnecessary.

The title speaks for itself. A header that does not contain declarations is usually unnecessary. There is no need to write a corresponding header (.hpp) for every implementation file (.cpp). A header is only needed if the implementation file actually provides some classes, functions or variables that need to be accessible in other translation units.

Enclosing namespaces of declaration and definition do not match.

Static class members, like:

namespace my_namespace {
  class my_class {
    // ...
    static int my_member;
    // ...
  };
}

Should be defined in the namespace of the class:

int my_namespace::my_class::my_member = 1;

Or:

namespace my_namespace {
  int my_class::my_member = 1;
}

Note: The global namespace and an anonymous namespace are generally not the same.

It is recommended to always initialize variables of built-in type.

Built-in types, like int are not default initialized to 0 in all contexts (Exercise: Which contexts?).

int a_variable; // Is this initialized or not?

If the initializer is omitted, it can easily lead to problems that are difficult to track down. Therefore it is generally preferable to always use an initializer:

int a_variable = 0; // This is definitely initialized.

Value-like parameter of class-type should preferably be a constant reference.

For efficiency reasons (sometimes also for correctness) it is generally better to make parameters of class type constant references.

// Implies copying, requires a copy-constructor:
void my_function(my_class my_parameter);

// No copying, no copy-constructor needed:
void my_function(const my_class& my_parameter);

Single parameter constructor should be explicit unless conversion is really needed.

In C++, a single parameter constructor defines an implicit conversion unless the constructor is marked explicit:

class my_class { // BAD IDEA
public:
  my_class(int x); // This defines a conversion from int to my_class
};

int some_function(const my_class& some_parameter);

int main() {
  some_function(5); // A my_class object is implicitly constructed from 5
}

Implicit conversions are, in many cases, surprising and can lead to ambiquities. Generally, one should make single parameter constructors explicit, unless there is a good reason to allow the implicit conversion.

				class my_class {
				public:
				    explicit my_class(int x);
				};

				int some_function(const my_class& some_parameter);

				int main() {
				     some_function(5); // Error
				     some_function(my_class(5)); // Ok
				}
			

Nullary constructors need not be marked explicit.

The title speaks for itself. A nullary or default constructor does not define an implicit conversion. Marking such constructors explicit is just waste of bits.

Copy constructor should not be marked explicit.

Well, a copy constructor does not define an implicit conversion that might be surprising or cause ambiquities. Marking such constructors explicit is just waste of bits.

Object created here is never deleted.

Standard C++ is not a garbage collected language. In general, objects created on memory allocated from the free store using the operator new must be destroyed and the memory freed by using the operator delete. Failure to delete objects created on the free store generally leads to two problems. First of all, the destructors of such objects are never called, which can cause resource leaks of many kind (e.g. open file descriptors, open network connections, thread handles, etc...). Second, the memory allocated to the objects is never freed, which can cause memory exhaustion. Therefore, all objects created on the free store should be deleted.

f(void) is a C-ism. Use f() in C++.

Here is what Stroustrup says in the book The Design and Evolution of C++:

"C with Classes introduced the notation f(void) for a function f that takes no arguments as a contrast to f() that in C declares a function that can take any number of arguments of any type without any type check. My users soon convinced me, however, that the f(void) notation wasn't elegant, and that having functions declared f() accept arguments wasn't intuitive. Consequently, the result of the experiment was to have f() mean a function f that takes no arguments, as any novice would expect. It took support from both Doug McIlroy and Dennis Ritchie for me to build up the courage to make this break from C. Only after they used the word abomination about f(void) did I dare give f() the obvious meaning."

Prefer ++var to var++ when possible.

The expressions ++var and var++ have important semantic differences. The form ++var is generally more efficient, because it does not imply copying like the form var++. It is good practise to learn away from var++ even when var refers to a variable of built-in type and isn't likely to be significantly less efficient. For user defined types, however, ++var is likely to be significantly more efficient than var++.

Virtual destructor is arguably unnecessary for this class.

As a rule of thumb, if a class contains a virtual function (other than the destructor) then that class should also have a virtual destructor. Likewise, if a class is intended as an abstract base class for deriving concrete classes, then a virtual destructor is likely to be appropriate. However, if a class isn't intended to function as a base class to be extended using derivation, then it may be more appropriate not to define a virtual destructor. As a rule of thumb, if a class does not define a virtual destructor, then it usually isn't a good idea to publicly derive from such a class.

Note that a class that has no virtual functions is likely to have a slightly more efficient representation. Basically, the so called virtual function table pointer can be omitted from the class. For classes that model datatypes like small dimensional vectors or a class such as a smart pointer, the optimization is likely to be very significant.

Variable by the name tmp is generally an indication of poorly understood code.

Well, any variable can seen as temporary. Naming a variable tmp reveals very little useful information about the intent and use the that particular variable. There really is only one exception:

template<class T>
			void swap(T& lhs, T& rhs) {
			     T tmp = lhs; // Justifiable, but not exemplary
			     lhs = rhs;
			     rhs = tmp;
			}

In a function as trivial as swap above, the use of a local variable named tmp can be justified, because the scope of the variable is so narrow that there isn't much chance for misunderstanding. However, even in this case it isn't too difficult to think of a clearly better name:

				template
				void swap(T& lhs, T& rhs) {
				   T copy_of_lhs = lhs;
				   lhs = rhs;
				   rhs = copy_of_lhs;
				}
			

At least to me, this version of swap is easier to understand.

Unused class declaration.

Consider the following code:

class string; // BAD IDEA

			void do_something(const std::string& str);
			

The above would seem to be an attempt to declare the class std::string, but it misses by several miles. First of all, std::string is not a class, it is a typedef. Second, the C++ standard does (unfortunately) not support so called forward declaration of class templates provided by the C++ standard library. This is because standard C++ allows standard library class templates to define additional template parameters.

On the other hand, it is quite error-prone to forward declare classes at the point in which they are used. In particular, it is easy to write redundant forward declarations that are never used (like above) or forget to change the forward declaration if the class name changes. It is therefore recommended to avoid such error-prone programming practice and use forward declaration headers, like the standard <iosfwd> -header.

A forward declaration header is a header that only contains forward declarations of classes, typedefs and possibly constant definitions. The compile time cost of including a forward declaration header should be small compared to the cost of including a normal header that contains full class definitions. Note that this recommendation may differ from the recommendations of the official course material (lecture notes/slides). I see the use of manually introduced class declarations at the point of use as an error-prone practise and do not support it.

Constants shouldn't be duplicated manually.

Just say no to magic constants:

world worlds[10]; // BAD IDEA
			for (int i=0; i<10; ++i)
			    worlds[i].say();
			

Do use a named constant

const int num_worlds = 10;
			world worlds[num_worlds];
			for (int i=0; i<num_worlds; ++i)
			  worlds[i].say();
			

A for-loop index should preferably be defined in the for-statement.

In 99 (or maybe 95) cases out of hundred, the value of a loop index is only interesting inside the loop. It is also generally good practise to limit the scope of variables to be as narrow as possible. Therefore it is generally recommended to define the index of a for-loop inside the for-statement:

for (int i = 0; i<n; ++i)
			  // ...

Some compilers (e.g. old MSVC++) might still not properly support the definition of variables in for-statements. You can "fix" those compilers by using the following (illegal!) macro:

#define for if (0) {} else for

Challenge: Explain why the above macro is illegal and explain how the macro actually works.

Please spread the definition of functions over multiple lines.

Sure, entire programs could be written on just one line. It is just not very smart to do so, because it would make it considerably more difficult to read the program. However, the request isn't really referring to readability. I make the request, because when you write a complete function definition on a single line, it makes it more difficult for me to write comments referring to different parts of the function definition. If this request is not honored, I may refuse to grade your program (and give it a straight 0 points). At minimum, the parameter list, the initializer list and every statement of the function body should be on separate lines. (OTOH, don't spread the definition of functions over too many lines. Use good judgement.)

Uninformative comments should be avoided.

What is wrong with the following comments?

// a class
			class world {
			    // the default constructor
			    world();

			    // an unary constructor
			    world(const std::string& what);

			    // the destructor
			    ~world();

			    // ...
			};

Well, the above comments (and I'm not talking about the last one) just don't tell anything that isn't already immediately obvious from the code. Don't write comments like above.

Unnecessary semicolon.

class world {
			    // ...
			    ~world() {};
			    // ...
			};
			

In other words, no semicolon is needed after a function definition. It just looks ugly.

main() should report exceptions.

Exceptions that escape from main will be caught by the C++ implementation and result in a call of std::terminate, which, by default, calls abort. Whether or not any useful diagnostic is given is not specified in the C++ standard. Therefore, main should catch and report all exceptions before the progam is terminated (preferably, if possible, by returning from main so that all objects will be destroyed).

std::endl might be more appropriate here than '\n'.

Well, std::endl flushes the stream causing any buffered output to be actually written (and '\n' doesn't). To be honest, I don't think that one should use std::endl always, but it can help debugging, for instance, because it might happen that important output might be buffered and never written before the program crashes. On the other hand, not all of the standard streams are buffered. (Exercise: Which standard streams are buffered and which are not?)

Violation of assignment/project requirements. See reason below.

I use this comment to point out violations of project requirements. The specific violation/reason varies. The following line in your program code should explain the specific reason. If there is no explanation, please ask me for one (it might be an error on my part and might affect your points).

Default argument definitions belong to the declaration.

In C++, a function declaration may contain default arguments. The default arguments may be repeated in subsequent declarations, but it isn't required. If the default arguments are repeated, then they must always be the same. The least confusing way to deal with default arguments is to specify them only once in the first declaration of a function in a header. Specifying default arguments for an interface function in an implementation file is usually a sign of a problem.

Unnecessary qualification.

In C++ one can use the scope resolution operator :: to explicitly qualify the namespace or class of a name. When used to qualify a class member, the scope resolution operator has the semantic that the member referred to is specifically a member of the named class rather than any member (virtual or otherwise) of a derived class. This can be useful for a) micro -optimization, b) in conjunction with multiple inheritance and c) in certain template programming techniques. However, using the scope resolution operator for "improving the readability" of code is not advisable, due to the semantic meaning of the operator. If you want to show that a particular name refers to a class member, then use a naming convention that uses different decorations for class members and variables.

It isn't necessary to use separate header & implementation files for an insulated implementation.

In both commonly used total insulation techniques (protocol class and pimpl-idiom) the completely hidden implementation can be placed in a single implementation file (.cpp). After all, the whole point of insulation is to insulate the clients from the implementation details and the hidden implementation is not supposed to be used directly. Therefore there is no need to provide a header through which the hidden implementation could be used.

By the way, I generally recommend using protocol classes instead of the pimpl-idiom when there is no particular reason to prefer one technique over the other. The reason for this is that the protocol class technique makes the use of many design patterns (e.g. Decorator, Null Object) more straightforward due to the availability of a pure interface. Another reason is that using protocol classes, one typically needs to write less forwarding code.

Unnecessary "clearing" of variables in destructors is not recommended.

The short answer is that clearing variables in destructors can actually hide bugs and that there are better less intrusive (require less code to be written) techniques for exposing similar bugs. A longer treat is given in this case study (in Finnish).

A header should not contain definitions of variables.

A header may contain declarations of variables:

// In a header file
			extern int a_global_var;

(This is one of the rare places where extern is required.) In most cases, if a header defines a variable, it is a sign of an error:

// BAD IDEAS in a header file
			int a_global_var;
			int another_global_var = 0;
			static int a_translation_unit_local_var;
			static int another_translation_unit_local_var = 76;

There are certain special techniques which use definitions of translation unit local variables, but that is the rare exception rather than the rule. (See section 21.5.2 Closing of Streams on page 639 of The C++ Programming Language, 3rd. ed. or section 3.11.4.2 Workarounds for Order Dependencies on page 97 of The Design and Evolution of C++.)

Destructor missing causing a resource leak

A class whose constructor or member functions allocate resources and store such resource handles in class members, should generally either use a automatic resource handle (a smart pointer) or should contain a destructor that frees the resources. It is generally preferable to use automatic resource handles and the RAII-principle, because correct implementation of resource management can be difficult without them.

class my_class { // BAD IDEA
			public:
			    my_class() : my_ptr(new int(5)) {}
			    // No destructor
			private:
			    int *my_ptr;
			};

Default assignment operator or copy constructor is inapproriate

In C++, a default assignment operator and a copy constructor are implicitly generated for a class under certain circumstances. The default assignment operator performs member-wise assignment and the default copy constructor performs member-wise copy construction. It is not unusual, particularly when a class manages resources of some kind (e.g. contains a pointer to an object allocated from the free store), that the implicitly generated assignment operator and copy constructor are dangerous.

class my_class { // BAD IDEA
			public:
			    my_class() : my_ptr(new int(5)) {}
			    // A default assignment operator and a copy constructor will be generated
			    ~my_class() { delete my_ptr; }
			private:
			    int *my_ptr;
			};

			int main() {
			    my_class x; // Internally allocates an integer from the free store

			    { my_class y( x ); // uses the same integer

				    // At the end of scope, the destructor of y is called, freeing the integer.
			  }
			    // At this point, x contains a dangling pointer.

			    // KABOOM! Destructor of x is now called, which attempts to
			    // free the integer again, which invokes undefined behavior.
			}

Similar programming errors are associated with the default assignment operator.


The following is a TODO-list of comments that haven't yet been explained. I will gradually add explanations for these comments.

** COMMENT: Either the instructions/manual is incorrect or the program is.
			** COMMENT: Build-script is missing.
			** COMMENT: No build/test -instructions provided.
			** COMMENT: Build-instructions are incorrect. (e.g. required compiler options are missing.)
			** COMMENT: A test-script should preferably be provided.
			** COMMENT: README-file was missing.
			# COMMENT: Required compiler options are missing.
			# COMMENT: Script contains an error.
			# COMMENT: Test-script should automatically determine whether the program behaves incorrectly or not.
			  // COMMENT: Inconsistent naming convention. Variables of same kind should have similar decorations.
			  // COMMENT: A non-mutating member function should be declared const.
			  // COMMENT: Implementation files (.cpp) should not normally be targets of an #include.
			  // COMMENT: Initializer list should be preferred to assignment.
			  // COMMENT: Duplication of code should be avoided.
			  // COMMENT: Arrays should be processed using loops.
			  // COMMENT: Identifiers that start with an underscore followed by a capital letter are reserved.
			  // COMMENT: Identifiers that contain a double underscore are reserved.
			  // COMMENT: Use of <using namespace> at global scope in a header is strongly discouraged.
			  // COMMENT: Implementation details should be encapsulated.
			  // COMMENT: Insulation is probably an overkill here.
			  // COMMENT: Use of smart pointers (RAII-principle) should be preferred to explicit delete.
			  // COMMENT: Initialization should be preferred to assignment.
			  // COMMENT: Private accessor is probably an overkill here.
			  // COMMENT: Out-commented code should preferably be removed from source files.
			  // COMMENT: Use of <using namespace> is generally discouraged.
			  // COMMENT: Indentation should be used to improve readability.
			  // COMMENT: Header <> should preferably be #included here.
			  // COMMENT: This declaration is unnecessary.
			  // COMMENT: Filenames should be treated case sensitively.
			  // COMMENT: Private would be more appropriate here.
			  // COMMENT: Indentation looks inconsistent in my editor. Please use only spaces and not tabs.
			  // COMMENT: The name of a side-effecting function should preferably be command-like.
			  // COMMENT: Object destruction should generally be done in reverse order of construction.
			  // COMMENT: Class and instance specific behavior should preferably be separated.
			  // COMMENT: Global variables should be avoided.
			  // COMMENT: This is unused.
			  // COMMENT: #includes should generally come before any declarations or definitions.
			  // COMMENT: Use of namespaces in this case is probably an overkill.
			  // COMMENT: This is not the proper definition file for this variable.
			  // COMMENT: This solution is overall an overkill considering the requirements.
			
Vesa Karvonen

Last modified: Fri Oct 22 05:43:19 EEST 2004

Re-styled by Daniel Zhang