Development/Tutorials/Common Programming Mistakes: Difference between revisions

From KDE TechBase
m (Mark for translation)
(Marked this version for translation)
Line 1: Line 1:
<languages/>
<languages/>
<translate>
<translate>
<!--T:1-->
{{TutorialBrowser|
{{TutorialBrowser|
series=Getting Started|
series=Getting Started|
Line 7: Line 8:
}}
}}


== Abstract ==
== Abstract == <!--T:2-->


<!--T:3-->
This tutorial aims to combine the experience of KDE developers regarding Qt and KDE frameworks dos and don'ts. Besides actual mistakes, it also covers things that are not necessarily "bugs" but which make the code either slower or less readable.
This tutorial aims to combine the experience of KDE developers regarding Qt and KDE frameworks dos and don'ts. Besides actual mistakes, it also covers things that are not necessarily "bugs" but which make the code either slower or less readable.


== General C++ ==
== General C++ == <!--T:4-->


<!--T:5-->
This section guides you through some of the more dusty corners of C++ which either tend to be misused or which people often simply get wrong.
This section guides you through some of the more dusty corners of C++ which either tend to be misused or which people often simply get wrong.


=== Anonymous namespaces vs statics ===
=== Anonymous namespaces vs statics === <!--T:6-->


<!--T:7-->
If you have a method in a class that does not access any members and therefore does not need an object to operate, make it static. If additionally, it is a private helper function that is not needed outside of the file, make it a file-static function. That hides the symbol completely.
If you have a method in a class that does not access any members and therefore does not need an object to operate, make it static. If additionally, it is a private helper function that is not needed outside of the file, make it a file-static function. That hides the symbol completely.


<!--T:8-->
Symbols defined in a C++ anonymous namespace do not have internal linkage. Anonymous namespaces only give a unique name for that translation unit and that is it; they do not change the linkage of the symbol at all. Linkage is not changed on those because the second phase of two-phase name lookup ignores functions with internal linkages. Also, entities with internal linkage cannot be used as template arguments.  
Symbols defined in a C++ anonymous namespace do not have internal linkage. Anonymous namespaces only give a unique name for that translation unit and that is it; they do not change the linkage of the symbol at all. Linkage is not changed on those because the second phase of two-phase name lookup ignores functions with internal linkages. Also, entities with internal linkage cannot be used as template arguments.  


<!--T:9-->
So for now instead of using anonymous namespaces use static if you do not want a symbol to be exported.
So for now instead of using anonymous namespaces use static if you do not want a symbol to be exported.


=== NULL pointer issues ===
=== NULL pointer issues === <!--T:10-->


<!--T:11-->
First and foremost: it is fine to delete a null pointer. So constructs like this that check for null before deleting are simply redundant:  
First and foremost: it is fine to delete a null pointer. So constructs like this that check for null before deleting are simply redundant:  
</translate>
</translate>
Line 33: Line 40:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
<!--T:12-->
Note however, that '''a null check ''is'' required when you delete an array''' - that's because a relatively recent compiler on Solaris does not handle it properly otherwise.
Note however, that '''a null check ''is'' required when you delete an array''' - that's because a relatively recent compiler on Solaris does not handle it properly otherwise.


<!--T:13-->
When you delete a pointer, make sure you also set it to 0 so that future attempts to delete that object will not fail in a double delete. So the complete and proper idiom is:  
When you delete a pointer, make sure you also set it to 0 so that future attempts to delete that object will not fail in a double delete. So the complete and proper idiom is:  
</translate>
</translate>
Line 42: Line 51:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
<!--T:14-->
You may notice that null pointers are marked variously in one of four ways: 0, 0L, NULL and nullptr. In C, NULL is defined as a null void pointer.  In C++, more type safety is possible due to stricter type checking.  Modern C++11 implementations (and all C++14 implementations) define NULL to equal the special value nullptr. Nullptr can be automatically cast to boolean false, but a cast to an integer type will fail.  This is useful to avoid accidentally.  Older C++ implementations before c++11 simply defined NULL to 0L or 0, which provides no additional type safety - one could assign it to an integer variable, which is obviously wrong. For code which does not need to support outdated compilers the best choice is nullptr.
You may notice that null pointers are marked variously in one of four ways: 0, 0L, NULL and nullptr. In C, NULL is defined as a null void pointer.  In C++, more type safety is possible due to stricter type checking.  Modern C++11 implementations (and all C++14 implementations) define NULL to equal the special value nullptr. Nullptr can be automatically cast to boolean false, but a cast to an integer type will fail.  This is useful to avoid accidentally.  Older C++ implementations before c++11 simply defined NULL to 0L or 0, which provides no additional type safety - one could assign it to an integer variable, which is obviously wrong. For code which does not need to support outdated compilers the best choice is nullptr.


<!--T:15-->
In pointer context, the integer constant zero means "null pointer" - irrespective of the actual binary representation of a null pointer. Note, however, that if you want to pass a null pointer constant to a function in a variable argument list, you *must* explicitly cast it to a pointer - the compiler assumes integer context by default, which might or might not match the binary representation of a pointer.
In pointer context, the integer constant zero means "null pointer" - irrespective of the actual binary representation of a null pointer. Note, however, that if you want to pass a null pointer constant to a function in a variable argument list, you *must* explicitly cast it to a pointer - the compiler assumes integer context by default, which might or might not match the binary representation of a pointer.


=== Member variables ===
=== Member variables === <!--T:16-->


<!--T:17-->
You will encounter four major styles of marking class member variables in KDE, besides unmarked members:
You will encounter four major styles of marking class member variables in KDE, besides unmarked members:


<!--T:18-->
* '''m_variable''' lowercase m, underscore and the name of the variable starting with a lowercase letter. This is the most common style and one preferred for code in kdelibs.
* '''m_variable''' lowercase m, underscore and the name of the variable starting with a lowercase letter. This is the most common style and one preferred for code in kdelibs.


<!--T:19-->
* '''mVariable''' lowercase m and the name of variable starting with an uppercase letter
* '''mVariable''' lowercase m and the name of variable starting with an uppercase letter


<!--T:20-->
* '''variable_''' variable name starting with a lowercase letter and then an underscore
* '''variable_''' variable name starting with a lowercase letter and then an underscore


<!--T:21-->
* '''_variable''' underscore and the name of variable starting with a lowercase letter. This style is actually usually frowned upon as this notation is also used in some code for function parameters instead.
* '''_variable''' underscore and the name of variable starting with a lowercase letter. This style is actually usually frowned upon as this notation is also used in some code for function parameters instead.


<!--T:22-->
Unmarked members are more common in the case of classes that use [[Policies/Library Code Policy#D-Pointers|d-pointers]].
Unmarked members are more common in the case of classes that use [[Policies/Library Code Policy#D-Pointers|d-pointers]].


<!--T:23-->
As it often happens there is no one correct way of doing it, so remember to always follow the syntax used by the application/library to which you are committing. If you're creating a new file, you may want to follow the coding style of the library or module you're adding the file to.
As it often happens there is no one correct way of doing it, so remember to always follow the syntax used by the application/library to which you are committing. If you're creating a new file, you may want to follow the coding style of the library or module you're adding the file to.


<!--T:24-->
Note that symbols starting with undercores are reserved to the C library (underscore followed by capital or double underscore are reserved to the compiler), so if you can, avoid using the last type.
Note that symbols starting with undercores are reserved to the C library (underscore followed by capital or double underscore are reserved to the compiler), so if you can, avoid using the last type.


=== Static variables ===
=== Static variables === <!--T:25-->


<!--T:26-->
Try to limit the number of static variables used in your code, especially when committing to a library. Construction and initialization of a large number of static variables really hurts the startup times.
Try to limit the number of static variables used in your code, especially when committing to a library. Construction and initialization of a large number of static variables really hurts the startup times.


<!--T:27-->
Do not use class-static variables, especially not in libraries and loadable modules though it is even discouraged in applications. Static objects lead to lots of problems such as hard to debug crashes due to undefined order of construction/destruction.
Do not use class-static variables, especially not in libraries and loadable modules though it is even discouraged in applications. Static objects lead to lots of problems such as hard to debug crashes due to undefined order of construction/destruction.


<!--T:28-->
Instead, use a static pointer, together with <tt>K_GLOBAL_STATIC</tt> which is defined in <tt>kglobal.h</tt> and is used like this:
Instead, use a static pointer, together with <tt>K_GLOBAL_STATIC</tt> which is defined in <tt>kglobal.h</tt> and is used like this:
</translate>
</translate>
Line 98: Line 120:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
<!--T:29-->
See the [http://api.kde.org/4.x-api/kdelibs-apidocs/kdecore/html/group__KDEMacros.html#ga75ca0c60b03dc5e4f9427263bf4043c7 API documentation] for <tt>K_GLOBAL_STATIC</tt> for more information.
See the [http://api.kde.org/4.x-api/kdelibs-apidocs/kdecore/html/group__KDEMacros.html#ga75ca0c60b03dc5e4f9427263bf4043c7 API documentation] for <tt>K_GLOBAL_STATIC</tt> for more information.


=== Constant data ===
=== Constant data === <!--T:30-->


<!--T:31-->
If you need some constant data of simple data types in several places, you do good by defining it once at a central place, to avoid a mistype in one of the instances. If the data changes there is also only one place you need to edit.
If you need some constant data of simple data types in several places, you do good by defining it once at a central place, to avoid a mistype in one of the instances. If the data changes there is also only one place you need to edit.


<!--T:32-->
Even if there is only one instance you do good by defining it elsewhere, to avoid so-called "magic numbers" in the code which are unexplained (cmp. 42). Usually this is done at the top of a file to avoid searching for it.
Even if there is only one instance you do good by defining it elsewhere, to avoid so-called "magic numbers" in the code which are unexplained (cmp. 42). Usually this is done at the top of a file to avoid searching for it.


<!--T:33-->
Define the constant data using the language constructs of C++, not the preprocessor instructions, like you may be used to from plain C. This way the compiler can help you to find mistakes by doing type checking.
Define the constant data using the language constructs of C++, not the preprocessor instructions, like you may be used to from plain C. This way the compiler can help you to find mistakes by doing type checking.


<!--T:34-->
<syntaxhighlight lang="cpp-qt">
<syntaxhighlight lang="cpp-qt">
// Correct!
// Correct!
Line 116: Line 143:




<!--T:35-->
If defining a constant array do not use a pointer as data type. Instead use the data type and append the array symbol with undefined length, <tt>[]</tt>, behind the name. Otherwise you also define a variable to some const data. That variable could mistakenly be assigned a new pointer to, without the compiler complaining about. And accessing the array would have one indirection, because first the value of the variable needs to be read.
If defining a constant array do not use a pointer as data type. Instead use the data type and append the array symbol with undefined length, <tt>[]</tt>, behind the name. Otherwise you also define a variable to some const data. That variable could mistakenly be assigned a new pointer to, without the compiler complaining about. And accessing the array would have one indirection, because first the value of the variable needs to be read.


<!--T:36-->
<syntaxhighlight lang="cpp-qt">
<syntaxhighlight lang="cpp-qt">
// Correct!
// Correct!
Line 127: Line 156:
</syntaxhighlight>
</syntaxhighlight>


=== Forward Declarations ===
=== Forward Declarations === <!--T:37-->


<!--T:38-->
You will reduce compile times by forward declaring classes when possible instead of including their respective headers. The rules for when a type can be used without being defined are a bit subtle, but intuitively, if the only important aspect is the name of the class, not the details of its implementation, a forward declaration is permissible. Two examples are when declaring pointers to the class or using the class as a function argument.   
You will reduce compile times by forward declaring classes when possible instead of including their respective headers. The rules for when a type can be used without being defined are a bit subtle, but intuitively, if the only important aspect is the name of the class, not the details of its implementation, a forward declaration is permissible. Two examples are when declaring pointers to the class or using the class as a function argument.   


<!--T:39-->
For example:  
For example:  
</translate>
</translate>
Line 149: Line 180:
</syntaxhighlight>   
</syntaxhighlight>   
<translate>
<translate>
<!--T:40-->
The above should instead be written like this:
The above should instead be written like this:
</translate>
</translate>
Line 167: Line 199:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
=== Iterators ===
=== Iterators === <!--T:41-->


==== Prefer const iterators and cache end() ====
==== Prefer const iterators and cache end() ==== <!--T:42-->


<!--T:43-->
Prefer to use <tt>const_iterators</tt> over normal iterators when possible. Containers, which are being implicitly shared often detach when a call to a non-const <tt>begin()</tt> or <tt>end()</tt> methods is made ({{qt|QList}} is an example of such a container). When using a const_iterator also watch out that you are really calling the const version of <tt>begin()</tt> and <tt>end()</tt>. Unless your container is actually const itself this probably will not be the case, possibly causing an unnecessary detach of your container. So basically whenever you use const_iterator initialize them using <tt>constBegin()</tt>/<tt>constEnd()</tt> instead, to be on the safe side.  
Prefer to use <tt>const_iterators</tt> over normal iterators when possible. Containers, which are being implicitly shared often detach when a call to a non-const <tt>begin()</tt> or <tt>end()</tt> methods is made ({{qt|QList}} is an example of such a container). When using a const_iterator also watch out that you are really calling the const version of <tt>begin()</tt> and <tt>end()</tt>. Unless your container is actually const itself this probably will not be the case, possibly causing an unnecessary detach of your container. So basically whenever you use const_iterator initialize them using <tt>constBegin()</tt>/<tt>constEnd()</tt> instead, to be on the safe side.  


<!--T:44-->
Cache the return of the <tt>end()</tt> (or <tt>constEnd()</tt>) method call before doing iteration over large containers. For example:
Cache the return of the <tt>end()</tt> (or <tt>constEnd()</tt>) method call before doing iteration over large containers. For example:
</translate>
</translate>
Line 188: Line 222:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
<!--T:45-->
This avoids the unnecessary creation of the temporary <tt>end()</tt> (or <tt>constEnd()</tt>) return object on each loop iteration, largely speeding it up.
This avoids the unnecessary creation of the temporary <tt>end()</tt> (or <tt>constEnd()</tt>) return object on each loop iteration, largely speeding it up.


<!--T:46-->
When using iterators, always use pre-increment and pre-decrement operators (i.e., <tt>++itr</tt>) unless you have a specific reason not to. The use of post-increment and post-decrement operators (i.e., <tt>itr++</tt>) cause the creation of a temporary object.
When using iterators, always use pre-increment and pre-decrement operators (i.e., <tt>++itr</tt>) unless you have a specific reason not to. The use of post-increment and post-decrement operators (i.e., <tt>itr++</tt>) cause the creation of a temporary object.


====Take care when erasing elements inside a loop====
====Take care when erasing elements inside a loop==== <!--T:47-->


<!--T:48-->
When you want to erase some elements from the list, you maybe would use code similar to this:
When you want to erase some elements from the list, you maybe would use code similar to this:
</translate>
</translate>
Line 209: Line 246:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
<!--T:49-->
This code will potentially crash because it is a dangling iterator after the call to erase().
This code will potentially crash because it is a dangling iterator after the call to erase().
You have to rewrite the code this way:
You have to rewrite the code this way:
Line 225: Line 263:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
<!--T:50-->
This problem is also discussed in the [https://doc.qt.io/qt-5/qmap-iterator.html#details Qt documentation for QMap::iterator] but applies to '''all''' Qt iterators
This problem is also discussed in the [https://doc.qt.io/qt-5/qmap-iterator.html#details Qt documentation for QMap::iterator] but applies to '''all''' Qt iterators


=== memory leaks ===
=== memory leaks === <!--T:51-->


<!--T:52-->
A very "popular" programming mistake is to do a <tt>new</tt> without a <tt>delete</tt> like in this program:
A very "popular" programming mistake is to do a <tt>new</tt> without a <tt>delete</tt> like in this program:
</translate>
</translate>
Line 250: Line 290:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
<!--T:53-->
You see, ''pollute()'' instantiates a new object ''polluter'' of the class ''t''. Then, the variable ''polluter'' is lost because it is local, but the content (the object) stays on the heap. I could use this program to render my computer unusable within 10 seconds.
You see, ''pollute()'' instantiates a new object ''polluter'' of the class ''t''. Then, the variable ''polluter'' is lost because it is local, but the content (the object) stays on the heap. I could use this program to render my computer unusable within 10 seconds.


<!--T:54-->
To solve this, there are the following approaches:
To solve this, there are the following approaches:


<!--T:55-->
* keep the variable on the stack instead of the heap:
* keep the variable on the stack instead of the heap:
</translate>
</translate>
Line 260: Line 303:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
<!--T:56-->
would become
would become
</translate>
</translate>
Line 266: Line 310:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
<!--T:57-->
* delete polluter using the complementing function to new:
* delete polluter using the complementing function to new:
</translate>
</translate>
Line 272: Line 317:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
<!--T:58-->
* stop the polluter in an [http://en.cppreference.com/w/cpp/memory/unique_ptr] (which will automatically delete the polluter when returning from the method)
* stop the polluter in an [http://en.cppreference.com/w/cpp/memory/unique_ptr] (which will automatically delete the polluter when returning from the method)
</translate>
</translate>
Line 278: Line 324:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
<!--T:59-->
There's also std::shared_ptr and QSharedPointer. This is the generally preferred way to do it in modern C++; explicit memory management should be avoided when possible.
There's also std::shared_ptr and QSharedPointer. This is the generally preferred way to do it in modern C++; explicit memory management should be avoided when possible.


<!--T:60-->
Qt code involving QObject generally uses parent/child relations to free allocated memory; when constructing a QObject (e.g. a widget) it can be given a parent, and when the parent is deleted it deletes all its children. The parent is also set when you add a widget to a layout, for example.
Qt code involving QObject generally uses parent/child relations to free allocated memory; when constructing a QObject (e.g. a widget) it can be given a parent, and when the parent is deleted it deletes all its children. The parent is also set when you add a widget to a layout, for example.


<!--T:61-->
A tool to detect memory leaks like this is [[Development/Tools/Valgrind|Valgrind]].
A tool to detect memory leaks like this is [[Development/Tools/Valgrind|Valgrind]].


=== dynamic_cast ===
=== dynamic_cast === <!--T:62-->


<!--T:63-->
You can only dynamic_cast to type T from type T2 provided  
You can only dynamic_cast to type T from type T2 provided  
that:
that:


<!--T:64-->
* T is defined in a library you link to (you'd get a linker error if this isn't the case, since it won't find the vtable or RTTI info)
* T is defined in a library you link to (you'd get a linker error if this isn't the case, since it won't find the vtable or RTTI info)


<!--T:65-->
* T is "well-anchored" in that library. By "well-anchored" I mean that the vtable is not a COMMON symbol subject to merging at run-time by the dynamic linker. In other words, the first virtual member in the class definition must exist and not be inlined: it must be in a .cpp file.
* T is "well-anchored" in that library. By "well-anchored" I mean that the vtable is not a COMMON symbol subject to merging at run-time by the dynamic linker. In other words, the first virtual member in the class definition must exist and not be inlined: it must be in a .cpp file.


<!--T:66-->
* T and T2 are exported
* T and T2 are exported


<!--T:67-->
For instance, we've seen some hard-to-track problems in non-KDE C++ code we're linking with (I think NMM) because of that. It happened that:
For instance, we've seen some hard-to-track problems in non-KDE C++ code we're linking with (I think NMM) because of that. It happened that:


<!--T:68-->
* libphonon loads the NMM plugin
* libphonon loads the NMM plugin


<!--T:69-->
* NMM plugin links to NMM
* NMM plugin links to NMM


<!--T:70-->
* NMM loads its own plugins
* NMM loads its own plugins


<!--T:71-->
* NMM's own plugins link to NMM
* NMM's own plugins link to NMM


<!--T:72-->
Some classes in the NMM library did not have well-anchored vtables, so dynamic_casting failed inside the Phonon NMM plugin for objects created in the NMM's own plugins.
Some classes in the NMM library did not have well-anchored vtables, so dynamic_casting failed inside the Phonon NMM plugin for objects created in the NMM's own plugins.


== Program Design ==
== Program Design == <!--T:73-->


<!--T:74-->
In this section we will go over some common problems related to the design of Qt/KDE applications.
In this section we will go over some common problems related to the design of Qt/KDE applications.


=== Delayed Initialization ===
=== Delayed Initialization === <!--T:75-->


<!--T:76-->
Although the design of modern C++ applications can be very complex, application windows can be loaded and displayed to the user very quickly through the technique of [http://www.kdedevelopers.org/node/509 delayed initialization].  This technique is relatively straightforward and useful at all stages of an interactive program.
Although the design of modern C++ applications can be very complex, application windows can be loaded and displayed to the user very quickly through the technique of [http://www.kdedevelopers.org/node/509 delayed initialization].  This technique is relatively straightforward and useful at all stages of an interactive program.


<!--T:77-->
First, let us look at the standard way of initializing a KDE application:  
First, let us look at the standard way of initializing a KDE application:  
</translate>
</translate>
Line 334: Line 396:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
<!--T:78-->
Notice that <tt>window</tt> is created before the <tt>a.exec()</tt> call that starts the event loop. This implies that we want to avoid doing anything non-trivial in the top-level constructor, since it runs before we can even show the window.
Notice that <tt>window</tt> is created before the <tt>a.exec()</tt> call that starts the event loop. This implies that we want to avoid doing anything non-trivial in the top-level constructor, since it runs before we can even show the window.


<!--T:79-->
The solution is simple: we need to delay the construction of anything besides the GUI until after the event loop has started. Here is how the example class MainWindow's constructor could look to achieve this:
The solution is simple: we need to delay the construction of anything besides the GUI until after the event loop has started. Here is how the example class MainWindow's constructor could look to achieve this:


<!--T:80-->
<syntaxhighlight lang="cpp-qt">
<syntaxhighlight lang="cpp-qt">
MainWindow::MainWindow()
MainWindow::MainWindow()
Line 345: Line 410:
}
}


<!--T:81-->
void MainWindow::initGUI()
void MainWindow::initGUI()
{
{
Line 356: Line 422:
}
}


<!--T:82-->
void MainWindow::initObject()
void MainWindow::initObject()
{
{
Line 369: Line 436:
Using this technique may not buy you any overall time, but it makes your app ''seem'' quicker to the user who is starting it. This increased perceived responsiveness is reassuring for the user as they get quick feedback that the action of launching the app has succeeded.
Using this technique may not buy you any overall time, but it makes your app ''seem'' quicker to the user who is starting it. This increased perceived responsiveness is reassuring for the user as they get quick feedback that the action of launching the app has succeeded.


<!--T:83-->
When (and only when) the start up can not be made reasonably fast enough, consider using a {{class|KSplashScreen}}.
When (and only when) the start up can not be made reasonably fast enough, consider using a {{class|KSplashScreen}}.


== Data Structures ==
== Data Structures == <!--T:84-->


<!--T:85-->
In this section we will go over some of our most common pet-peeves which affect data structures very commonly seen in Qt/KDE applications.
In this section we will go over some of our most common pet-peeves which affect data structures very commonly seen in Qt/KDE applications.


=== Passing non-POD types ===
=== Passing non-POD types === <!--T:86-->


<!--T:87-->
Non-POD ("plain old data") types should be passed by const reference if at all possible. This includes anything other than the basic types such as <tt>char</tt> and <tt>int</tt>.
Non-POD ("plain old data") types should be passed by const reference if at all possible. This includes anything other than the basic types such as <tt>char</tt> and <tt>int</tt>.


<!--T:88-->
Take, for instance, {{qt|QString}}. They should always be passed into methods as <tt>const {{qt|QString}}&</tt>. Even though {{qt|QString}} is implicitly shared it is still more efficient (and safer) to pass const references as opposed to objects by value.  
Take, for instance, {{qt|QString}}. They should always be passed into methods as <tt>const {{qt|QString}}&</tt>. Even though {{qt|QString}} is implicitly shared it is still more efficient (and safer) to pass const references as opposed to objects by value.  


<!--T:89-->
So the canonical signature of a method taking QString arguments is:
So the canonical signature of a method taking QString arguments is:
</translate>
</translate>
Line 387: Line 459:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
=== QObject ===
=== QObject === <!--T:90-->


<!--T:91-->
If you ever need to delete a QObject derived class from within one of its own methods, do not ever delete it this way:  
If you ever need to delete a QObject derived class from within one of its own methods, do not ever delete it this way:  
</translate>
</translate>
Line 395: Line 468:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
<!--T:92-->
This will sooner or later cause a crash because a method on that object might be invoked from the Qt event loop via slots/signals after you deleted it.
This will sooner or later cause a crash because a method on that object might be invoked from the Qt event loop via slots/signals after you deleted it.


<!--T:93-->
Instead always use <tt>{{QtMethod|QObject|deleteLater}}</tt> which tries to do the same thing as <tt>delete this</tt> but in a safer way.
Instead always use <tt>{{QtMethod|QObject|deleteLater}}</tt> which tries to do the same thing as <tt>delete this</tt> but in a safer way.


=== Empty QStrings ===
=== Empty QStrings === <!--T:94-->


<!--T:95-->
It is common to want to see if a {{qt|QString}} is empty. Here are three ways of doing it, the first two of which are correct:
It is common to want to see if a {{qt|QString}} is empty. Here are three ways of doing it, the first two of which are correct:


<!--T:96-->
<syntaxhighlight lang="cpp-qt">
<syntaxhighlight lang="cpp-qt">
// Correct
// Correct
Line 408: Line 485:
}
}


<!--T:97-->
// Correct
// Correct
if ( mystring == QString() ) {
if ( mystring == QString() ) {
}
}


<!--T:98-->
// Wrong! ""
// Wrong! ""
if ( mystring == "" ) {
if ( mystring == "" ) {
Line 417: Line 496:
</syntaxhighlight>
</syntaxhighlight>


<!--T:99-->
While there is a distinction between "null" {{qt|QString}}s and empty ones, this is a purely historical artifact and new code is discouraged from making use of it.
While there is a distinction between "null" {{qt|QString}}s and empty ones, this is a purely historical artifact and new code is discouraged from making use of it.


=== QString and reading files ===
=== QString and reading files === <!--T:100-->


<!--T:101-->
If you are reading in a file, it is faster to convert it from the local encoding to Unicode ({{qt|QString}}) in one go, rather than line by line. This means that methods like <tt>{{qt|QIODevice}}::readAll()</tt> are often a good solution, followed by a single {{qt|QString}} instantiation.
If you are reading in a file, it is faster to convert it from the local encoding to Unicode ({{qt|QString}}) in one go, rather than line by line. This means that methods like <tt>{{qt|QIODevice}}::readAll()</tt> are often a good solution, followed by a single {{qt|QString}} instantiation.


<!--T:102-->
For larger files, consider reading a block of lines and then performing the conversion. That way you get the opportunity to update your GUI. This can be accomplished by reentering the event loop normally, along with using a timer to read in the blocks in the background, or by creating a local event loop.  
For larger files, consider reading a block of lines and then performing the conversion. That way you get the opportunity to update your GUI. This can be accomplished by reentering the event loop normally, along with using a timer to read in the blocks in the background, or by creating a local event loop.  


<!--T:103-->
While one can also use <tt>qApp->processEvents()</tt>, it is discouraged as it easily leads to subtle yet often fatal problems.
While one can also use <tt>qApp->processEvents()</tt>, it is discouraged as it easily leads to subtle yet often fatal problems.


=== Reading QString from a KProcess ===
=== Reading QString from a KProcess === <!--T:104-->


<!--T:105-->
{{class|KProcess}} emits the signals <tt>readyReadStandard{Output|Error}</tt> as data comes in.
{{class|KProcess}} emits the signals <tt>readyReadStandard{Output|Error}</tt> as data comes in.
A common mistake is reading all available data in the connected slot and converting it to {{qt|QString}} right away: the data comes in arbitrarily segmented chunks, so multi-byte characters might be cut into pieces and thus invalidated. Several approaches to this problem exist:
A common mistake is reading all available data in the connected slot and converting it to {{qt|QString}} right away: the data comes in arbitrarily segmented chunks, so multi-byte characters might be cut into pieces and thus invalidated. Several approaches to this problem exist:


<!--T:106-->
* Do you really need to process the data as it comes in? If not, just use <tt>readAllStandard{Output|Error}</tt> after the process has exited. Unlike in KDE3, KProcess is now able to accumulate the data for you.
* Do you really need to process the data as it comes in? If not, just use <tt>readAllStandard{Output|Error}</tt> after the process has exited. Unlike in KDE3, KProcess is now able to accumulate the data for you.


<!--T:107-->
* Wrap the process into a {{qt|QTextStream}} and read line-wise. This should work starting with Qt 4.4.
* Wrap the process into a {{qt|QTextStream}} and read line-wise. This should work starting with Qt 4.4.


<!--T:108-->
* Accumulate data chunks in the slots and process them each time a newline arrives or after some timeout passes. [http://websvn.kde.org/trunk/KDE/kdevplatform/util/processlinemaker.cpp?view=markup Example code]
* Accumulate data chunks in the slots and process them each time a newline arrives or after some timeout passes. [http://websvn.kde.org/trunk/KDE/kdevplatform/util/processlinemaker.cpp?view=markup Example code]


=== QString and QByteArray ===
=== QString and QByteArray === <!--T:109-->


<!--T:110-->
While {{qt|QString}} is the tool of choice for many string handling situations, there is one where it is particularly inefficient. If you are pushing about and working on data in {{qt|QByteArray}}s, take care not to pass it through methods which take {{qt|QString}} parameters; then make QByteArrays from them again.
While {{qt|QString}} is the tool of choice for many string handling situations, there is one where it is particularly inefficient. If you are pushing about and working on data in {{qt|QByteArray}}s, take care not to pass it through methods which take {{qt|QString}} parameters; then make QByteArrays from them again.


<!--T:111-->
For example:  
For example:  
</translate>
</translate>
Line 455: Line 544:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
<!--T:112-->
The expensive thing happening here is the conversion to {{qt|QString}}, which does a conversion to Unicode internally. This is unnecessary because, the first thing the method does is convert it back using <tt>toLatin1()</tt>. So if you are sure that the Unicode conversion is not needed, try to avoid inadvertently using QString along the way.  
The expensive thing happening here is the conversion to {{qt|QString}}, which does a conversion to Unicode internally. This is unnecessary because, the first thing the method does is convert it back using <tt>toLatin1()</tt>. So if you are sure that the Unicode conversion is not needed, try to avoid inadvertently using QString along the way.  


<!--T:113-->
The above example should instead be written as:
The above example should instead be written as:
</translate>
</translate>
Line 466: Line 557:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
=== QDomElement ===
=== QDomElement === <!--T:114-->


<!--T:115-->
When parsing XML documents, one often needs to iterate over all the elements. You may be tempted to use the following code for that:  
When parsing XML documents, one often needs to iterate over all the elements. You may be tempted to use the following code for that:  
</translate>
</translate>
Line 478: Line 570:
</syntaxhighlight>
</syntaxhighlight>
<translate>
<translate>
<!--T:116-->
That is not correct though: the above loop will stop prematurely when it encounters a {{qt|QDomNode}} that is something other than an element such as a comment.
That is not correct though: the above loop will stop prematurely when it encounters a {{qt|QDomNode}} that is something other than an element such as a comment.


<!--T:117-->
The correct loop looks like:  
The correct loop looks like:  
</translate>
</translate>

Revision as of 18:20, 25 October 2019

Common Programming Mistakes
Tutorial Series   Getting Started
Previous   None
What's Next   n/a
Further Reading   APIs to avoid

Abstract

This tutorial aims to combine the experience of KDE developers regarding Qt and KDE frameworks dos and don'ts. Besides actual mistakes, it also covers things that are not necessarily "bugs" but which make the code either slower or less readable.

General C++

This section guides you through some of the more dusty corners of C++ which either tend to be misused or which people often simply get wrong.

Anonymous namespaces vs statics

If you have a method in a class that does not access any members and therefore does not need an object to operate, make it static. If additionally, it is a private helper function that is not needed outside of the file, make it a file-static function. That hides the symbol completely.

Symbols defined in a C++ anonymous namespace do not have internal linkage. Anonymous namespaces only give a unique name for that translation unit and that is it; they do not change the linkage of the symbol at all. Linkage is not changed on those because the second phase of two-phase name lookup ignores functions with internal linkages. Also, entities with internal linkage cannot be used as template arguments.

So for now instead of using anonymous namespaces use static if you do not want a symbol to be exported.

NULL pointer issues

First and foremost: it is fine to delete a null pointer. So constructs like this that check for null before deleting are simply redundant:

if ( ptr ) {
   delete ptr;
}

Note however, that a null check is required when you delete an array - that's because a relatively recent compiler on Solaris does not handle it properly otherwise.

When you delete a pointer, make sure you also set it to 0 so that future attempts to delete that object will not fail in a double delete. So the complete and proper idiom is:

delete ptr; 
ptr = 0;

You may notice that null pointers are marked variously in one of four ways: 0, 0L, NULL and nullptr. In C, NULL is defined as a null void pointer. In C++, more type safety is possible due to stricter type checking. Modern C++11 implementations (and all C++14 implementations) define NULL to equal the special value nullptr. Nullptr can be automatically cast to boolean false, but a cast to an integer type will fail. This is useful to avoid accidentally. Older C++ implementations before c++11 simply defined NULL to 0L or 0, which provides no additional type safety - one could assign it to an integer variable, which is obviously wrong. For code which does not need to support outdated compilers the best choice is nullptr.

In pointer context, the integer constant zero means "null pointer" - irrespective of the actual binary representation of a null pointer. Note, however, that if you want to pass a null pointer constant to a function in a variable argument list, you *must* explicitly cast it to a pointer - the compiler assumes integer context by default, which might or might not match the binary representation of a pointer.

Member variables

You will encounter four major styles of marking class member variables in KDE, besides unmarked members:

  • m_variable lowercase m, underscore and the name of the variable starting with a lowercase letter. This is the most common style and one preferred for code in kdelibs.
  • mVariable lowercase m and the name of variable starting with an uppercase letter
  • variable_ variable name starting with a lowercase letter and then an underscore
  • _variable underscore and the name of variable starting with a lowercase letter. This style is actually usually frowned upon as this notation is also used in some code for function parameters instead.

Unmarked members are more common in the case of classes that use d-pointers.

As it often happens there is no one correct way of doing it, so remember to always follow the syntax used by the application/library to which you are committing. If you're creating a new file, you may want to follow the coding style of the library or module you're adding the file to.

Note that symbols starting with undercores are reserved to the C library (underscore followed by capital or double underscore are reserved to the compiler), so if you can, avoid using the last type.

Static variables

Try to limit the number of static variables used in your code, especially when committing to a library. Construction and initialization of a large number of static variables really hurts the startup times.

Do not use class-static variables, especially not in libraries and loadable modules though it is even discouraged in applications. Static objects lead to lots of problems such as hard to debug crashes due to undefined order of construction/destruction.

Instead, use a static pointer, together with K_GLOBAL_STATIC which is defined in kglobal.h and is used like this:

class A { ... };

K_GLOBAL_STATIC(A, globalA)

void doSomething()
{
     A *a = globalA;
     ...
}

void doSomethingElse()
{
    if (globalA.isDestroyed()) {
        return;
    }
    A *a = globalA;
    ...
}

void installPostRoutine()
{
    qAddPostRoutine(globalA.destroy);
}

See the API documentation for K_GLOBAL_STATIC for more information.

Constant data

If you need some constant data of simple data types in several places, you do good by defining it once at a central place, to avoid a mistype in one of the instances. If the data changes there is also only one place you need to edit.

Even if there is only one instance you do good by defining it elsewhere, to avoid so-called "magic numbers" in the code which are unexplained (cmp. 42). Usually this is done at the top of a file to avoid searching for it.

Define the constant data using the language constructs of C++, not the preprocessor instructions, like you may be used to from plain C. This way the compiler can help you to find mistakes by doing type checking.

// Correct!
static const int AnswerToAllQuestions = 42;
// Wrong!
#define AnswerToAllQuestions 42


If defining a constant array do not use a pointer as data type. Instead use the data type and append the array symbol with undefined length, [], behind the name. Otherwise you also define a variable to some const data. That variable could mistakenly be assigned a new pointer to, without the compiler complaining about. And accessing the array would have one indirection, because first the value of the variable needs to be read.

// Correct!
static const char SomeString[] = "Example";
// Wrong!
static const char* SomeString = "Example";
// Wrong!
#define SomeString "Example"

Forward Declarations

You will reduce compile times by forward declaring classes when possible instead of including their respective headers. The rules for when a type can be used without being defined are a bit subtle, but intuitively, if the only important aspect is the name of the class, not the details of its implementation, a forward declaration is permissible. Two examples are when declaring pointers to the class or using the class as a function argument.

For example:

#include <QWidget>     // slow
#include <QStringList> // slow
#include <QString>     // slow
#include <QIcon>      //slow
class SomeClass
{
public:
    virtual void widgetAction( QWidget *widget ) =0;
    virtual void stringAction( const QString& str ) =0;
    virtual void stringListAction( const QStringList& strList ) =0;
private: 
    QIcon *icon;
};

The above should instead be written like this:

class QWidget;     // fast
class QStringList; // fast
class QString;     // fast
class QIcon;      // fast
class SomeClass
{
public:
    virtual void widgetAction( QWidget *widget ) =0;
    virtual void stringAction( const QString& str ) =0;
    virtual void stringListAction( const QStringList& strList ) =0;
private: 
    QIcon *icon;
};

Iterators

Prefer const iterators and cache end()

Prefer to use const_iterators over normal iterators when possible. Containers, which are being implicitly shared often detach when a call to a non-const begin() or end() methods is made (QList is an example of such a container). When using a const_iterator also watch out that you are really calling the const version of begin() and end(). Unless your container is actually const itself this probably will not be the case, possibly causing an unnecessary detach of your container. So basically whenever you use const_iterator initialize them using constBegin()/constEnd() instead, to be on the safe side.

Cache the return of the end() (or constEnd()) method call before doing iteration over large containers. For example:

QList<SomeClass> container;

//code which inserts a large number of elements to the container

QList<SomeClass>::ConstIterator end = container.constEnd();
QList<SomeClass>::ConstIterator itr = container.constBegin();

for ( ; itr != end; ++itr ) {
    // use *itr (or itr.value()) here
}

This avoids the unnecessary creation of the temporary end() (or constEnd()) return object on each loop iteration, largely speeding it up.

When using iterators, always use pre-increment and pre-decrement operators (i.e., ++itr) unless you have a specific reason not to. The use of post-increment and post-decrement operators (i.e., itr++) cause the creation of a temporary object.

Take care when erasing elements inside a loop

When you want to erase some elements from the list, you maybe would use code similar to this:

QMap<int, Job *>::iterator it = m_activeTimers.begin();
QMap<int, Job *>::iterator itEnd = m_activeTimers.end();

for( ; it != itEnd ; ++it) {
    if(it.value() == job) {
        //A timer for this job has been found. Let's stop it.
        killTimer(it.key());
        m_activeTimers.erase(it);
    }
}

This code will potentially crash because it is a dangling iterator after the call to erase(). You have to rewrite the code this way:

QMap<int, Job *>::iterator it = m_activeTimers.begin();
while (it != m_activeTimers.end()) {
    if(it.value() == job) {
        //A timer for this job has been found. Let's stop it.
        killTimer(it.key());
        it = m_activeTimers.erase(it);
    } else {
        ++it;
    }
}

This problem is also discussed in the Qt documentation for QMap::iterator but applies to all Qt iterators

memory leaks

A very "popular" programming mistake is to do a new without a delete like in this program: mem_gourmet.cpp

class t
{
    public:
      t() {}
};

void pollute()
{
    t* polluter = new t();
}

int main()
{
    while (true) pollute();
}

You see, pollute() instantiates a new object polluter of the class t. Then, the variable polluter is lost because it is local, but the content (the object) stays on the heap. I could use this program to render my computer unusable within 10 seconds.

To solve this, there are the following approaches:

  • keep the variable on the stack instead of the heap:
t* polluter = new t();

would become

 
t polluter;
  • delete polluter using the complementing function to new:
 
delete polluter;
  • stop the polluter in an [1] (which will automatically delete the polluter when returning from the method)
 std::unique_ptr<t> polluter = new t();

There's also std::shared_ptr and QSharedPointer. This is the generally preferred way to do it in modern C++; explicit memory management should be avoided when possible.

Qt code involving QObject generally uses parent/child relations to free allocated memory; when constructing a QObject (e.g. a widget) it can be given a parent, and when the parent is deleted it deletes all its children. The parent is also set when you add a widget to a layout, for example.

A tool to detect memory leaks like this is Valgrind.

dynamic_cast

You can only dynamic_cast to type T from type T2 provided that:

  • T is defined in a library you link to (you'd get a linker error if this isn't the case, since it won't find the vtable or RTTI info)
  • T is "well-anchored" in that library. By "well-anchored" I mean that the vtable is not a COMMON symbol subject to merging at run-time by the dynamic linker. In other words, the first virtual member in the class definition must exist and not be inlined: it must be in a .cpp file.
  • T and T2 are exported

For instance, we've seen some hard-to-track problems in non-KDE C++ code we're linking with (I think NMM) because of that. It happened that:

  • libphonon loads the NMM plugin
  • NMM plugin links to NMM
  • NMM loads its own plugins
  • NMM's own plugins link to NMM

Some classes in the NMM library did not have well-anchored vtables, so dynamic_casting failed inside the Phonon NMM plugin for objects created in the NMM's own plugins.

Program Design

In this section we will go over some common problems related to the design of Qt/KDE applications.

Delayed Initialization

Although the design of modern C++ applications can be very complex, application windows can be loaded and displayed to the user very quickly through the technique of delayed initialization. This technique is relatively straightforward and useful at all stages of an interactive program.

First, let us look at the standard way of initializing a KDE application:

int main( int argc, char **argv )
{
    ....
    KApplication a;

    KCmdLineArgs *args = KCmdLineArgs::parsedArgs();

    MainWindow *window = new MainWindow( args );

    a.setMainWidget( window );
    window->show();

    return a.exec();
}

Notice that window is created before the a.exec() call that starts the event loop. This implies that we want to avoid doing anything non-trivial in the top-level constructor, since it runs before we can even show the window.

The solution is simple: we need to delay the construction of anything besides the GUI until after the event loop has started. Here is how the example class MainWindow's constructor could look to achieve this:

MainWindow::MainWindow()
{
    initGUI();
    QTimer::singleShot( 0, this, SLOT(initObject()) );
}

void MainWindow::initGUI()
{
    /* Construct your widgets here.  Note that the widgets you
     * construct here shouldn't require complex initialization
     * either, or you've defeated the purpose.
     * All you want to do is create your GUI objects and
     * QObject::connect
     * the appropriate signals to their slots.
     */
}

void MainWindow::initObject()
{
    /* This slot will be called as soon as the event loop starts.
     * Put everything else that needs to be done, including
     * restoring values, reading files, session restoring, etc here.
     * It will still take time, but at least your window will be
     * on the screen, making your app look active.
     */
}

Using this technique may not buy you any overall time, but it makes your app seem quicker to the user who is starting it. This increased perceived responsiveness is reassuring for the user as they get quick feedback that the action of launching the app has succeeded.

When (and only when) the start up can not be made reasonably fast enough, consider using a KSplashScreen.

Data Structures

In this section we will go over some of our most common pet-peeves which affect data structures very commonly seen in Qt/KDE applications.

Passing non-POD types

Non-POD ("plain old data") types should be passed by const reference if at all possible. This includes anything other than the basic types such as char and int.

Take, for instance, QString. They should always be passed into methods as const QString&. Even though QString is implicitly shared it is still more efficient (and safer) to pass const references as opposed to objects by value.

So the canonical signature of a method taking QString arguments is:

void myMethod( const QString & foo, const QString & bar );

QObject

If you ever need to delete a QObject derived class from within one of its own methods, do not ever delete it this way:

delete this;

This will sooner or later cause a crash because a method on that object might be invoked from the Qt event loop via slots/signals after you deleted it.

Instead always use QObject::deleteLater() which tries to do the same thing as delete this but in a safer way.

Empty QStrings

It is common to want to see if a QString is empty. Here are three ways of doing it, the first two of which are correct:

// Correct
if ( mystring.isEmpty() ) {
}

// Correct
if ( mystring == QString() ) {
}

// Wrong! ""
if ( mystring == "" ) {
}

While there is a distinction between "null" QStrings and empty ones, this is a purely historical artifact and new code is discouraged from making use of it.

QString and reading files

If you are reading in a file, it is faster to convert it from the local encoding to Unicode (QString) in one go, rather than line by line. This means that methods like QIODevice::readAll() are often a good solution, followed by a single QString instantiation.

For larger files, consider reading a block of lines and then performing the conversion. That way you get the opportunity to update your GUI. This can be accomplished by reentering the event loop normally, along with using a timer to read in the blocks in the background, or by creating a local event loop.

While one can also use qApp->processEvents(), it is discouraged as it easily leads to subtle yet often fatal problems.

Reading QString from a KProcess

KProcess emits the signals readyReadStandard{Output|Error} as data comes in. A common mistake is reading all available data in the connected slot and converting it to QString right away: the data comes in arbitrarily segmented chunks, so multi-byte characters might be cut into pieces and thus invalidated. Several approaches to this problem exist:

  • Do you really need to process the data as it comes in? If not, just use readAllStandard{Output|Error} after the process has exited. Unlike in KDE3, KProcess is now able to accumulate the data for you.
  • Wrap the process into a QTextStream and read line-wise. This should work starting with Qt 4.4.
  • Accumulate data chunks in the slots and process them each time a newline arrives or after some timeout passes. Example code

QString and QByteArray

While QString is the tool of choice for many string handling situations, there is one where it is particularly inefficient. If you are pushing about and working on data in QByteArrays, take care not to pass it through methods which take QString parameters; then make QByteArrays from them again.

For example:

QByteArray myData;
QString myNewData = mangleData( myData );

QString mangleData( const QString& data ) {
    QByteArray str = data.toLatin1();
    // mangle 
    return QString(str);
}

The expensive thing happening here is the conversion to QString, which does a conversion to Unicode internally. This is unnecessary because, the first thing the method does is convert it back using toLatin1(). So if you are sure that the Unicode conversion is not needed, try to avoid inadvertently using QString along the way.

The above example should instead be written as:

QByteArray myData;
QByteArray myNewData = mangleData( myData );

QByteArray mangleData( const QByteArray& data )

QDomElement

When parsing XML documents, one often needs to iterate over all the elements. You may be tempted to use the following code for that:

for ( QDomElement e = baseElement.firstChild().toElement();
      !e.isNull();
      e = e.nextSibling().toElement() ) {
       ...
}

That is not correct though: the above loop will stop prematurely when it encounters a QDomNode that is something other than an element such as a comment.

The correct loop looks like:

for ( QDomNode n = baseElement.firstChild(); !n.isNull();
      n = n.nextSibling() ) {
    QDomElement e = n.toElement();
    if ( e.isNull() ) {
        continue;
    }
    ...
}