Development/Tutorials/Common Programming Mistakes: Difference between revisions

From KDE TechBase
m (fixed typo)
(Moved)
Tag: Replaced
 
(44 intermediate revisions by 19 users not shown)
Line 1: Line 1:
{{TutorialBrowser|
Moved to https://develop.kde.org/docs/getting-started/common-programming-mistakes/
 
series=Getting Started|
 
name=Common Programming Mistakes|
 
}}
 
== 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 which 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:
 
<code cppqt>
if ( ptr ) {
  delete ptr;
}
</code>
 
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:
 
<code cppqt>
delete ptr;
ptr = 0;
</code>
 
You may notice that null pointers are marked variously in one of three ways: 0, 0L and NULL. The argument against using NULL was that while C defines it as a 0 void pointer, C++ defines it to not be a 0 void pointer. All conforming C++ implementations will define NULL correctly so it is really not a problem. The argument for 0L was that it was handled correctly in variable argument functions, while 0 was not. Nowadays that is also an artifact.
 
It is more a question of personal style and getting used to something. As far as the code in KDE's SVN goes you will see 0 used more commonly than NULL.
 
=== Member variables ===
 
You will encounter four major styles of marking class member variables in KDE:
 
* '''m_variable''' lowercase m, underscore and the name of the variable starting with a lowercase letter. This is the most common style and one prefered for code in kdelibs.
* '''mVariable''' lowercase m and the name of variable starting with a 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.
 
As it often happens there is not one correct way of doing it, so remember to always follow the syntax used by the application/library to which you are committing.
 
=== Static variables ===
 
Try to limit the number of static variables used in your code, especially when committing to a library. Construction and initialization of 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 <tt>K_GLOBAL_STATIC</tt> which is defined in <tt>kglobal.h</tt> and is used like this:
 
<code cppqt>
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);
}
</code>
 
See the [http://www.englishbreakfastnetwork.org/apidocs/apidox-kde-4.0/kdelibs-apidocs/kdecore/html/kglobal_8h.html#75ca0c60b03dc5e4f9427263bf4043c7 API documentation] for <tt>K_GLOBAL_STATIC</tt> for more information.
 
=== Forward Declarations ===
 
You will reduce compile times by forward declaring classes when possible instead of including their respective headers. For example:
 
<code cppqt>
#include <QWidget>    // slow
#include <QStringList> // slow
#include <QString>    // slow
class SomeInterface
{
public:
    virtual void widgetAction( QWidget *widget ) =0;
    virtual void stringAction( const QString& str ) =0;
    virtual void stringListAction( const QStringList& strList ) =0;
};
</code> 
 
The above should instead be written like this:
 
<code cppqt>
class QWidget;    // fast
class QStringList; // fast
class QString;    // fast
class SomeInterface
{
public:
    virtual void widgetAction( QWidget *widget ) =0;
    virtual void stringAction( const QString& str ) =0;
    virtual void stringListAction( const QStringList& strList ) =0;
};
</code>
 
=== Iterators ===
 
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|List}} 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.
 
Cache the return of the <tt>end()</tt> method call before doing iteration over large containers. For example:
 
<code cppqt>
QValueList<SomeClass> container;
 
//code which inserts a large number of elements to the container
 
QValueListConstIterator end( container.end() );
 
for ( QValueListConstIterator itr( container.begin() );
    itr != end; ++itr ) {
}
</code>
 
This avoids the unnecessary creation of the temporary <tt>end()</tt> return object on each loop iteration, largely speeding it up.
 
Prefer to use pre-increment over post-increment operators on iterators as this avoids creating an unnecessary temporary object in the process.
 
'''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:
 
<code cppqt>
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);
    }
}
</code>
 
This code will potentially crash because it is a dangling iterator after the call to erase().
You have to rewrite the code this way:
<code cppqt>
QMap<int, Job *>::iterator it = m_activeTimers.begin();
while (it != m_activeTimers.end())
{
    QMap<int, Job *>::iterator prev = it;
    ++it;
    if(prev.value() == job)
    {
        //A timer for this job has been found. Let's stop it.
        killTimer(prev.key());
        m_activeTimers.erase(prev);
    }
}
</code>
This problem is also discussed in the [http://doc.trolltech.com/4.3/qmap-iterator.html#details Qt documentation for QMap::iterator] but applies to '''all''' Qt iterators
 
== 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, one recurring problem, which is generally easy to fix, is not using the technique of [http://www.kdedevelopers.org/node/view/509 delayed initialization].
 
First, let us look at the standard way of initializing a KDE application:
 
<code cppqt>
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();
}
</code>
   
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.
 
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:
 
<code cppqt>
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.
    */
}
</code>
 
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 {{class|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 <tt>char</tt> and <tt>int</tt>.
 
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.
 
So the canonical signature of a method taking QString arguments is:
 
<code cppqt>
void myMethod( const QString & foo, const QString & bar );
</code>
 
=== 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:
 
<code cppqt>
  delete this;
</code>
 
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 <tt>{{qt|QObject}}::deleteLater()</tt> which tries to do the same thing as <tt>delete this</tt> but in a safer way.
 
=== Empty QStrings ===
 
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:
 
<code cppqt>
// Correct
if ( mystring.isEmpty() ) {
}
 
// Correct
if ( mystring == QString() ) {
}
 
// Wrong! ""
if ( mystring == "" ) {
}
</code>
 
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 ===
 
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.
 
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 <tt>qApp->processEvents()</tt>, it is discouraged as it easily leads to subtle yet often fatal problems.
 
=== QString and QByteArray ===
 
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.
 
For example:
 
<code cppqt>
QCString myData;
QString myNewData = mangleData( myData );
 
QString mangleData( const QString data ) {
    QCString str = data.toLatin1();
    // mangle
    return QString(str);
}
</code>
   
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 above example should instead be written as:
 
<code cppqt>
QCString myData;
QCString myNewData = mangleData( myData );
 
QCString mangleData( const QCString& data )
</code>
 
=== 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:
 
<code cppqt>
for ( QDomElement e = baseElement.firstChild().toElement();
      !e.isNull();
      e = e.nextSibling().toElement() ) {
      ...
}
</code>
 
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.
 
The correct loop looks like:
 
<code cppqt>
for ( QDomNode n = baseElement.firstChild(); !n.isNull();
      n = n.nextSibling() ) {
    QDomElement e = n.toElement();
    if ( e.isNull() ) {
        continue;
    }
    ...
}
</code>

Latest revision as of 12:00, 13 September 2023

Moved to https://develop.kde.org/docs/getting-started/common-programming-mistakes/

Retrieved from "https://techbase.kde.org/index.php?title=Development/Tutorials/Common_Programming_Mistakes&oldid=107003"