Checking the Qt 5 Framework

18.04.2014 Andrey Karpov

Static code analysis tools can help developers eliminate numbers of bugs as early as at the coding stage. With their help you can, for example, quickly catch and fix any typos. Well, some programmers are sincerely sure they never make typos and silly mistakes. But they are wrong; everyone makes mistakes. This article is a good evidence of that. Typos can be found even in high-quality and well tested projects such as Qt.

Picture 4

Qt

Qt is a cross-platform application framework that is widely used for developing application software with a graphical user interface (GUI) (in which cases Qt is classified as a widget toolkit), and also used for developing non-GUI programs such as command-line tools and consoles for servers.

Qt uses standard C++ but makes extensive use of a special code generator (called the Meta Object Compiler, or moc) together with several macros to enrich the language. Qt can also be used in several other programming languages via language bindings. It runs on the major desktop platforms and some of the mobile platforms. It has extensive internationalization support. Non-GUI features include SQL database access, XML parsing, thread management, network support, and a unified cross-platform application programming interface (API) for file handling. [the source: Wikipedia]

Links:

This time we were dealing with Qt 5.2.1. Analysis was done with the PVS-Studio 5.15 analyzer.

Please note that PVS-Studio managed to detect bugs despite that the Qt project had been checked before by the Klocwork and Coverity analyzers. I don't know how regularly the project's authors use these tools, but Klocwork and Coverity are mentioned in the bugtracker and ChangeLog-xxx files. I also saw Qt mentioned to be regularly checked with PC-lint.

Specifics of the Qt project's analysis

Just for a change, we decided to check Qt using a new mechanism recently introduced in PVS-Studio Standalone. Nobody knows of this mechanism yet, so we will remind you about it from time to time in our next articles. Well, what is that mysterious and wonderful mechanism, after all?

In certain cases, you may have a difficult time trying to check a project with PVS-Studio - these are the cases when the project is built with nmake and the like. You need to integrate PVS-Studio into the build, which becomes not an easy thing to do. To say the least, quickly trying and making up an opinion about the tool will become impossible.

But now PVS-Studio has acquired a new mode which makes it much simpler to work with such projects. The analyzer has learned how to trace compilation parameters and collect all the necessary information for analysis. You just need to tell the analyzer when to start monitoring compiler calls and when to stop it.

Compilation monitoring can be controlled both from the GUI application and the command line. To find out more about how it all works and how to use this mode, see the following article:

Evgeniy Ryzhkov. PVS-Studio Now Supports Any Build System under Windows and Any Compiler. Easy and Right Out of the Box.

It describes the process of checking the Qt project with the monitoring mode launched from the command line.

Do read it please to avoid any misconceptions. For example, you should keep in mind that you can't code while project compilation is being monitored: if you compile files from another project, the analyzer will collect the information about these files and check them too. It will result in the analysis report including extraneous messages, the relevant and irrelevant warnings all mixed in one pile.

Analysis results

My general opinion of Qt's code is this:

It is pretty high-quality and is almost free of bugs related to dangerous specifics of the C++ language. On the other hand, it does have quite a lot of ordinary typos.

This article is a good illustration of the thesis that every developer makes typos, however skillful he is. Static code analysis has always been and will be topical and useful. Suppose the analyzer has found 10 typos with a one-time check. So it could have prevented hundreds or thousands of bugs by now if it had been used regularly. That makes an enormous amount of time that could have been saved. Therefore it is much more profitable to detect an error right after it has been made than at the stage of code debugging or after user complaints.

Welcome to a wondrous world of typos

Picture 3

Typo No.1

bool QWindowsUser32DLL::initTouch()
{
  QSystemLibrary library(QStringLiteral("user32"));

  registerTouchWindow   = ....;
  unregisterTouchWindow = ....;
  getTouchInputInfo     = ....;
  closeTouchInputHandle = ....;

  return registerTouchWindow &&
         unregisterTouchWindow &&
         getTouchInputInfo &&
         getTouchInputInfo;
}

PVS-Studio's diagnostic message: V501 There are identical sub-expressions 'getTouchInputInfo' to the left and to the right of the '&&' operator. qwindowscontext.cpp 216

Values are assigned to four variables, and all the four must be checked. But only 3 are actually checked because of a typo. In the last line, 'closeTouchInputHandle' should be written instead of 'getTouchInputInfo'.

Typo No.2

QWindowsNativeImage *QWindowsFontEngine::drawGDIGlyph(....)
{
  ....
  int iw = gm.width.toInt();
  int ih = gm.height.toInt();
  if (iw <= 0 || iw <= 0)
    return 0;
  ....
}

PVS-Studio's diagnostic message: V501 There are identical sub-expressions to the left and to the right of the '||' operator: iw <= 0 || iw <= 0 qwindowsfontengine.cpp 1095

The check of the height parameter stored in the 'ih' variable is missing.

Typos No.3, No.4

This error was found inside tests. A nice example of how static analysis complements unit tests. To find out more on this topic, see the article: "How to complement TDD with static analysis".

inline bool qCompare(QImage const &t1, QImage const &t2, ....)
{
  ....
  if (t1.width() != t2.width() || t2.height() != t2.height()) {
  ....
}

PVS-Studio's diagnostic message: V501 There are identical sub-expressions to the left and to the right of the '!=' operator: t2.height() != t2.height() qtest_gui.h 101

The function to compare two images is incorrectly comparing their heights. Or rather, it doesn't compare them at all.

This bug was multiplied through the Copy-Paste method. The same comparison can be found a bit farther in the code in the same file (line 135).

Typo No.5

I apologize for the ugly code formatting - the lines were too lengthy.

void QXmlSimpleReader::setFeature(
  const QString& name, bool enable)
{
  ....
  } else if (   name == QLatin1String(
    "http://trolltech.com/xml/features/report-start-end-entity")
             || name == QLatin1String(
    "http://trolltech.com/xml/features/report-start-end-entity"))
  {
  ....
}

PVS-Studio's diagnostic message: V501 There are identical sub-expressions to the left and to the right of the '||' operator. qxml.cpp 3249

The 'name' variable is compared to one and the same string twice. A bit earlier in the code, a similar comparison can be found where a variable is compared with the following two strings:

  • http://trolltech.com/xml/features/report-whitespace-only-CharData
  • http://qt-project.org/xml/features/report-whitespace-only-CharData

By analogy, you can conclude that the 'name' variable in the fragment we are discussing should have been compared with the following strings:

  • http://trolltech.com/xml/features/report-start-end-entity
  • http://qt-project.org/xml/features/report-start-end-entity

Typos No.6, No.7, No.8, No.9

QString DayTimeDuration::stringValue() const
{
  ....
  if(!m_hours && !m_minutes && !m_seconds && !m_seconds)
  ....
}

PVS-Studio's diagnostic message: V501 There are identical sub-expressions '!m_seconds' to the left and to the right of the '&&' operator. qdaytimeduration.cpp 148

The programmer forgot about milliseconds. Milliseconds are stored in the 'm_mseconds' variable. The check should look like this:

if(!m_hours && !m_minutes && !m_seconds && !m_mseconds)

There are similar mistakes with milliseconds in three other fragments:

  • qdaytimeduration.cpp 170
  • qduration.cpp 167
  • qduration.cpp 189

Typo No.10

QV4::ReturnedValue
QQuickJSContext2DPrototype::method_getImageData(
  QV4::CallContext *ctx)
{
  ....
  qreal x = ctx->callData->args[0].toNumber();
  qreal y = ctx->callData->args[1].toNumber();
  qreal w = ctx->callData->args[2].toNumber();
  qreal h = ctx->callData->args[3].toNumber();
  if (!qIsFinite(x) || !qIsFinite(y) ||
      !qIsFinite(w) || !qIsFinite(w))
  ....
}

PVS-Studio's diagnostic message: V501 There are identical sub-expressions '!qIsFinite(w)' to the left and to the right of the '||' operator. qquickcontext2d.cpp 3305

A check of the 'h' variable is missing. The 'w' variable is checked twice instead.

Typo No.11

AtomicComparator::ComparisonResult
IntegerComparator::compare(const Item &o1,
                           const AtomicComparator::Operator,
                           const Item &o2) const
{
  const Numeric *const num1 = o1.as<Numeric>();
  const Numeric *const num2 = o1.as<Numeric>();
 
  if(num1->isSigned() || num2->isSigned())
  ....
}

V656 Variables 'num1', 'num2' are initialized through the call to the same function. It's probably an error or un-optimized code. Consider inspecting the 'o1.as < Numeric > ()' expression. Check lines: 220, 221. qatomiccomparators.cpp 221

The variables 'num1' and 'num2' are initialized to one and the same value. Then both the variables are checked, and that is strange: it would be enough to check only one variable.

The 'num2' variable was most likely meant to be initialized to an expression with the 'o2' argument:

const Numeric *const num1 = o1.as<Numeric>();
const Numeric *const num2 = o2.as<Numeric>();

Typo No.12

void Atlas::uploadBgra(Texture *texture)
{
  const QRect &r = texture->atlasSubRect();
  QImage image = texture->image();

  if (image.format() != QImage::Format_ARGB32_Premultiplied ||
      image.format() != QImage::Format_RGB32) {
  ....
}

V547 Expression is always true. Probably the '&&' operator should be used here. qsgatlastexture.cpp 271

The condition in this code is meaningless as it is always true. Here you are a simplified sample to make it clearer:

int a = ...;
if (a != 1 || a != 2)

The variable will always be not equal to something.

I can't say for sure what exactly the correct code should look like. It may be like this:

if (image.format() == QImage::Format_ARGB32_Premultiplied ||
    image.format() == QImage::Format_RGB32) {

or this:

if (image.format() != QImage::Format_ARGB32_Premultiplied &&
    image.format() != QImage::Format_RGB32) {

Typo No.13

void QDeclarativeStateGroupPrivate::setCurrentStateInternal(
  const QString &state, 
  bool ignoreTrans)
{
  ....
  QDeclarativeTransition *transition =
    (ignoreTrans || ignoreTrans) ?
      0 : findTransition(currentState, state);
  ....
}

PVS-Studio's diagnostic message: V501 There are identical sub-expressions to the left and to the right of the '||' operator: ignoreTrans || ignoreTrans qdeclarativestategroup.cpp 442

Something is wrong with this code. I can't figure out how exactly the programmer meant to implement the check.

Typo No.14

QV4::ReturnedValue
QQuickJSContext2DPrototype::method_createPattern(....)
{
  ....
  if (repetition == QStringLiteral("repeat") ||
      repetition.isEmpty()) {
    pattern->patternRepeatX = true;
    pattern->patternRepeatY = true;
  } else if (repetition == QStringLiteral("repeat-x")) {
    pattern->patternRepeatX = true;
  } else if (repetition == QStringLiteral("repeat-y")) {
    pattern->patternRepeatY = true;
  } else if (repetition == QStringLiteral("no-repeat")) {
    pattern->patternRepeatY = false;
    pattern->patternRepeatY = false;
  } else {
    //TODO: exception: SYNTAX_ERR
  }
  ....
}

PVS-Studio's diagnostic message: V519 The 'pattern->patternRepeatY' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1775, 1776. qquickcontext2d.cpp 1776

The 'patternRepeatY' variable is assigned values twice on end:

pattern->patternRepeatY = false;
pattern->patternRepeatY = false;

I guess the correct code should look as follows:

} else if (repetition == QStringLiteral("no-repeat")) {
  pattern->patternRepeatX = false;
  pattern->patternRepeatY = false;
} else {

Misuse of the C++ language

Picture 5

As I've already said, most bugs in this project are ordinary typos. There are almost no errors related to misuse of the C++ language. However, the analyzer has caught a couple of these.

A nice error related to operation priorities

bool QConfFileSettingsPrivate::readIniLine(....)
{
  ....
  char ch;
  while (i < dataLen &&
         ((ch = data.at(i) != '\n') && ch != '\r'))
    ++i;
  ....
}

V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. qsettings.cpp 1702

The loop is designed to find the end of a string. The characters '\n' or '\r' are used as end-of-string indicators.

Inside the condition, a character must be taken and compared against '\n' and '\r'. The error occurs because the '!=' operator's priority is higher than that of the '=' operator. Because of this, the 'true' or 'false' value is written instead of the character code into the 'ch' variable. It makes the '\r' comparison meaningless.

Let's arrange parentheses to make the error clearer:

while (i < dataLen &&
       ((ch = (data.at(i) != '\n')) && ch != '\r'))

Because of the mistake, it is only the '\n' character that is treated as an end-of-string indicator. The function won't work correctly for strings ending with '\r'.

The fixed code should look as follows:

while (i < dataLen &&
       (ch = data.at(i)) != '\n' && ch != '\r')

Loss of Accuracy

bool QWindowsTabletSupport::translateTabletPacketEvent()
{
  ....
  const double radAzim =
    (packet.pkOrientation.orAzimuth / 10) * (M_PI / 180);
  ....
}

V636 The 'packet.pkOrientation.orAzimuth / 10' expression was implicitly casted from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. qwindowstabletsupport.cpp 467

The 'packet.pkOrientation.orAzimuth' variable is of the 'int' type. This integer variable is divided by 10. What is suspicious about this is that the quotient is then used together with values of the 'double' type. The final result is also saved into a variable of the 'double' type.

Such integer division is not always an error. Perhaps this code is written just the way the programmer intended. But practice shows that it is more often than not a mistake causing accuracy loss.

Suppose, for instance, that the 'packet.pkOrientation.orAzimuth' variable equals 55. Then the calculation result will be:

(55 / 10) * (3.14159... / 180) = 5 * 0,01745... = 0,087266...

The accuracy of these calculations can be significantly improved by just declaring the 10 constant as of the double type: "(packet.pkOrientation.orAzimuth / 10.0) * (M_PI / 180)". The result will then be:

(55 / 10.0) * (3.14159... / 180) = 5.5 * 0,01745... = 0,095993...

Accuracy losses like that often happen because of programmers being careless about expressions where different types are used together. It is also due to this carelessness that many 64-bit errors occur (see mixed arithmetic).

The analyzer has found 51 more suspicious cases of integer division. Some of them may prove less accurate than the programmer wanted them to be. I've collected the corresponding diagnostic messages in a separate list: qt-v636.txt.

Meaningless pointer checks

It has been for a long time now that checking a pointer for being null doesn't make any sense when the 'new' operator is used to allocate memory. Nowadays, it throws an exception when it fails to allocate memory. Of course, you can make the 'new' operator return 0, but we are not speaking of these cases now.

However, programmers sometimes forget about that and write meaningless checks in their code.

HRESULT STDMETHODCALLTYPE QWindowsEnumerate::Clone(
  IEnumVARIANT **ppEnum)
{
  QWindowsEnumerate *penum = 0;
  *ppEnum = 0;
  
  penum = new QWindowsEnumerate(array);
  if (!penum)
    return E_OUTOFMEMORY;
  ....
}

PVS-Studio's diagnostic message: V668 There is no sense in testing the 'penum' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. qwindowsmsaaaccessible.cpp 141

There are some more checks like that in the project: main.cpp 127, qaudiodevicefactory.cpp 236, qaudiodevicefactory.cpp 263, qaudiobuffer.cpp 488, mfvideorenderercontrol.cpp 143, mfvideorenderercontrol.cpp 158, mfvideorenderercontrol.cpp 1193, mfvideorenderercontrol.cpp 1199, qaxserverbase.cpp 1006, positionpollfactory.cpp 60.

The dark side

Picture 1

There are two code fragments in the Qt project about which I can't say for sure if they are errors or not as I'm not familiar with the project architecture and its implementation specifics. But even if they don't have errors, they certainly belong to the dark side of the C++ programming practice.

class Q_CORE_EXPORT QObject
{
  ....
  virtual ~QObject();
  virtual bool event(QEvent *);
  virtual bool eventFilter(QObject *, QEvent *);
  ....
};

QObject *QQmlVME::run(....)
{
  ....
  QObject *o = (QObject *)operator
    new(instr.typeSize + sizeof(QQmlData));   
  ::memset(static_cast<void *>(o), 0,
           instr.typeSize + sizeof(QQmlData));
  ....
}

PVS-Studio's diagnostic message: V598 The 'memset' function is used to nullify the fields of 'QObject' class. Virtual method table will be damaged by this. qqmlvme.cpp 658

The QObject class has virtual functions, which means the object stores a pointer to a virtual methods table. I don't find it a good idea to implement such objects through the memset() function.

One more message of that kind: V598 The 'memset' function is used to nullify the fields of 'QObject' class. Virtual method table will be damaged by this. qdeclarativevme.cpp 286

Null pointer dereferencing

I guess these errors could be classified as typos, but I like to single them out into a separate group. It makes them look somewhat more somber and serious.

Note. Bug classification is pretty relative; many errors can be usually classified as a typo, a vulnerability, an array overrun, and so on.

But let's get back to null pointers.

A typo leading to null pointer dereferencing

QV4::ReturnedValue QQuickJSContext2DPixelData::getIndexed(
  QV4::Managed *m, uint index, bool *hasProperty)
{
  ....
  if (!m)
    return m->engine()->currentContext()->throwTypeError();
  ....
}

PVS-Studio's diagnostic message: V522 Dereferencing of the null pointer 'm' might take place. qquickcontext2d.cpp 3169

I'm sure the '!' operator is unnecessary here. It's an ordinary typo leading to a serious bug.

Null pointer dereferencing in an error handler

void QDocIndexFiles::readIndexSection(....)
{
  ....
  DocNode* dn = qdb_->findGroup(groupNames[i]);
  if (dn) {
    dn->addMember(node);
  }
  else {
    ....
    qDebug() << "DID NOT FIND GROUP:" << dn->name()
             << "for:" << node->name();
  }
  ....
}

PVS-Studio's diagnostic message: V522 Dereferencing of the null pointer 'dn' might take place. qdocindexfiles.cpp 539

If an error occurs, the program must print an error message trying to take the name from a nonexistent object: dn->name().

82 potential null pointer dereferencing errors

Most project (and Qt is no exception) have null pointer handling issues. The check is often done after the pointer has been used. It's not always an error; there are cases when the pointer just can never be null.

But anyway, such fragments need to be attentively checked and refactored. Even if there is no error, an excess pointer check confuses the programmer reading the code.

Have a look at one dangerous code sample:

static int gray_raster_render(....)
{
  const QT_FT_Outline* outline =
    (const QT_FT_Outline*)params->source;

  ....

  /* return immediately if the outline is empty */
  if ( outline->n_points == 0 || outline->n_contours <= 0 )
    return 0;

  if ( !outline || !outline->contours || !outline->points )
    return ErrRaster_Invalid_Outline;  

  ....
}

PVS-Studio's diagnostic message: V595 The 'outline' pointer was utilized before it was verified against nullptr. Check lines: 1746, 1749. qgrayraster.c 1746

I guess the error must have appeared when the programmer was trying to optimize the gray_raster_render() function. It seems that the following lines were added later into an already complete function code:

/* return immediately if the outline is empty */
if ( outline->n_points == 0 || outline->n_contours <= 0 )
  return 0;

The trouble is that the 'outline' pointer may be null, but the necessary check is written after that fragment.

The analyzer has found 81 more potential issues like that. Here is a complete list of them: qt-v595.txt.

Questions without answers

Picture 2

There are strange code fragments about whose origin and the programmer's intentions about them you can't be sure. They may be typos or incomplete code or unsuccessful refactoring - whatever.

Double check

QWindowsFontEngine::~QWindowsFontEngine()
{
  ....
  if (QWindowsContext::verboseFonts)
    if (QWindowsContext::verboseFonts)
      qDebug("%s: font='%s", __FUNCTION__, qPrintable(_name));
  ....
}

PVS-Studio's diagnostic message: V571 Recurring check. The 'if (QWindowsContext::verboseFonts)' condition was already verified in line 369. qwindowsfontengine.cpp 370

What's the use checking one and the same thing twice? One of the checks is probably excess; or something else was meant to be checked.

Double assignment

void Moc::parse()
{
  ....
  index = def.begin + 1;
  namespaceList += def;
  index = rewind;
  ....
}

PVS-Studio's diagnostic message: V519 The 'index' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 568, 570. moc.cpp 570

Why different values are assigned to the 'index' variable?

There are a few more similar strange code fragments:

  • V519 The 'exitCode' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 807, 815. qprocess.cpp 815
  • V519 The 'detecting' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 163, 164. qhoversensorgesturerecognizer.cpp 164
  • V519 The 'increaseCount' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 185, 186. qtwistsensorgesturerecognizer.cpp 186

Suspecting a missing 'break' operator

bool GSuggestCompletion::eventFilter(QObject *obj, QEvent *ev)
{
  ....
  switch (key) {
  case Qt::Key_Enter:
  case Qt::Key_Return:
    doneCompletion();
    consumed = true;

  case Qt::Key_Escape:
    editor->setFocus();
    popup->hide();
    consumed = true;

  case Qt::Key_Up:
  case Qt::Key_Down:
  case Qt::Key_Home:
  case Qt::Key_End:
  case Qt::Key_PageUp:
  case Qt::Key_PageDown:
    break;
  ....
}

PVS-Studio's diagnostic message: V519 The 'consumed' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 110, 115. googlesuggest.cpp 115

So is the break operator missing here or not?

The analyzer found it strange that the 'consumed' variable was assigned the 'true' value twice on end. It suggests a missing break operator, but I'm not sure. It may be just that the first assignment should be removed: "consumed = true;".

Suspecting an excess 'break' operator

bool QHelpGenerator::registerVirtualFolder(....)
{
  ....
  while (d->query->next()) {
    d->namespaceId = d->query->value(0).toInt();
    break;
  }
  ....
}

PVS-Studio's diagnostic message: V612 An unconditional 'break' within a loop. qhelpgenerator.cpp 429

Was the 'break' operator really meant to terminate the loop right away?

One more fragment of that kind can be found here: qhelpgenerator.cpp 642

Miscellaneous

Be patient: there's not much left, just a handful of diverse errors.

Incorrect use of the toLower() function

int main(int argc, char **argv)
{
  ....
  QByteArray arg(argv[a]);
  ....
  arg = arg.mid(1);
  arg.toLower();
  if (arg == "o")
  ....
}

PVS-Studio's diagnostic message: V530 The return value of function 'toLower' is required to be utilized. main.cpp 72

The 'toLower()' function doesn't change the object - it returns a copy of an object that will store lower case characters.

One more defect: V530 The return value of function 'toLower' is required to be utilized. main.cpp 1522

Array index out of bounds

It's a complicated issue, so please be attentive.

There is an enum type in the code:

typedef enum {
    JNone,
    JCausing,
    JDual,
    JRight,
    JTransparent
} Joining;

Note that JTransparent == 4 and keep that in mind.

Now let's examine the getNkoJoining() function:

static Joining getNkoJoining(unsigned short uc)
{
  if (uc < 0x7ca)
    return JNone;
  if (uc <= 0x7ea)
    return JDual;
  if (uc <= 0x7f3)
    return JTransparent;
  if (uc <= 0x7f9)
    return JNone;
  if (uc == 0x7fa)
    return JCausing;
  return JNone;
}

What matters to us is that this function may return 'JTransparent', i.e. the function may return 4.

There is also a two-dimensional array 'joining_table':

static const JoiningPair joining_table[5][4] = { .... };

And here's the piece of code itself where the error may occur:

static void getNkoProperties(....)
{
  ....
  Joining j = getNkoJoining(chars[0]);
  ArabicShape shape = joining_table[XIsolated][j].form2;
  ....
}

PVS-Studio's diagnostic message: V557 Array overrun is possible. The value of 'j' index could reach 4. harfbuzz-arabic.c 516

As we remember, the getNkoJoining() function may return 4. Thus, we will be addressing the array cell joining_table[...][4] in this case, which is illegal because an array overrun will occur.

Identical conditions

void Node::setPageType(const QString& t)
{
    if ((t == "API") || (t == "api"))
        pageType_ = ApiPage;
    else if (t == "howto")
        pageType_ = HowToPage;
    else if (t == "overview")
        pageType_ = OverviewPage;
    else if (t == "tutorial")
        pageType_ = TutorialPage;
    else if (t == "howto")
        pageType_ = HowToPage;
    else if (t == "article")
        pageType_ = ArticlePage;
    else if (t == "example")
        pageType_ = ExamplePage;
    else if (t == "ditamap")
        pageType_ = DitaMapPage;
}

PVS-Studio's diagnostic message: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 386, 392. node.cpp 386

The (t == "howto") check is executed twice. I guess one of the checks is not necessary.

Here are a couple of other similar warnings:

  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 188, 195. qmaintainingreader_tpl_p.h 188
  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 299, 303. mfmetadatacontrol.cpp 299

Identical branches are executed

void
QBluetoothServiceDiscoveryAgentPrivate::_q_deviceDiscovered(
  const QBluetoothDeviceInfo &info)
{
  if(mode == QBluetoothServiceDiscoveryAgent::FullDiscovery) {
    for(int i = 0; i < discoveredDevices.count(); i++){
      if(discoveredDevices.at(i).address() == info.address()){
        discoveredDevices.removeAt(i);
      }
    }
    discoveredDevices.prepend(info);
  }
  else {
    for(int i = 0; i < discoveredDevices.count(); i++){
      if(discoveredDevices.at(i).address() == info.address()){
        discoveredDevices.removeAt(i);
      }
    }
    discoveredDevices.prepend(info);
  }
}

PVS-Studio's diagnostic message: V523 The 'then' statement is equivalent to the 'else' statement. qbluetoothservicediscoveryagent.cpp 402

Regardless of the condition, one and the same code branch is executed.

Other similar defects: pcre_exec.c 5577, ditaxmlgenerator.cpp 1722, htmlgenerator.cpp 388.

Inherited errors

Qt employs a few third-party libraries. Those also contain errors, therefore they can be said to belong to Qt as well. I decided not to describe them in the article, but I should mention them at least.

I didn't study the reports for the libraries attentively, but I have noted down some bugs: qt-3rdparty.txt.

Note. Don't assume, however, that I was attentively studying bugs from Qt instead. The project is pretty large and even a superficial analysis was enough to collect examples for this article.

Conclusions

PVS-Studio is an excellent, powerful analyzer capable of catching bugs even in high-quality and cleaned up projects such as the Qt framework.

It can help a developer team save huge amounts of time by revealing many bugs at the earliest development stage. With the incremental analysis mode enabled, errors will be detected immediately after compilation.

References

  • We regularly check open-source projects. For example: Tor, Chromium, Clang, Firebird, OpenCV. All those interested are welcome: "Updatable List of Open-Source Projects Checked with PVS-Studio".
  • Here you can download the PVS-Studio trial version. For a start you are given 20 clicks to navigate through the diagnostic messages. After submitting your personal information, you are granted 200 more clicks.
  • Things are much simpler with the lightweight CppCat analyzer. This tool is an excellent solution for small teams and single developers. The trial version is fully functional for 7 days. The license costs $250 and can be renewed at $200. We also offer discounts for purchasing several copies at once.