Check of the Krita 4.0 Open Source Graphics Editor

Egor Bredikhin
Articles: 6



Not so long ago, a new version of the Krita 4.0 free graphics editor was released. It's high time to check this project using PVS-Studio.

Picture 1

Introduction

It's quite remarkable, that developers have already used PVS-Studio in far 2015 for the version Krita 2.9.2 and have successfully fixed the bugs with the help of it. However, it seems like since that time they haven't used the analyzer. In many of our articles, we often say that regular checks are really important, because if the developers had continued using PVS-Studio, the errors, which I'm considering in this article, simply wouldn't have got in the release.

Useless Range-based for

PVS-Studio warning: V714 Variable row is not passed into for each loop by a reference, but its value is changed inside of the loop. kis_algebra_2d.cpp 532

DecomposedMatix::DecomposedMatix(const QTransform &t0)
{
    ....
    if (!qFuzzyCompare(t.m33(), 1.0)) {
        const qreal invM33 = 1.0 / t.m33();

        for (auto row : rows) { // <=
            row *= invM33;
        }
    }
    ....
}

In this example, a programmer obviously wanted to multiply each element of the container rows by invM33, however, this will not happen. On each iteration of the loop, a new variable with the name row is created and then simply removed. To correct this error, you must create a reference to an element stored in a container:

for (auto &row : rows) {
    row *= invM33;
}

Incorrect Conditions

PVS-Studio warning: V547 Expression 'j == 0' is always false. KoColorSpace.cpp 218

QPolygonF KoColorSpace::estimatedTRCXYY() const
{
  ....
  for (int j = 5; j>0; j--) {
    channelValuesF.fill(0.0);
    channelValuesF[i] = ((max / 4)*(5 - j));

    if (colorModelId().id() != "XYZA") {
      fromNormalisedChannelsValue(data, channelValuesF);
      convertPixelsTo(....);
      xyzColorSpace->normalisedChannelsValue(....);
    }

    if (j == 0) {                                 // <=
      colorantY = channelValuesF[1];
      if (d->colorants.size()<2) {
        d->colorants.resize(3 * colorChannelCount());
        d->colorants[i] = ....
          d->colorants[i + 1] = ....
          d->colorants[i + 2] = ....
      }
    }
  }
  ....
}

A program will never go in the block under the condition j==0, because this condition is always false due to the fact that earlier in the for cycle there is a limitation j > 0.

Similar analyzer warnings:

  • V547 Expression 'x < 0' is always false. kis_selection_filters.cpp 334
  • V547 Expression 'y < 0' is always false. kis_selection_filters.cpp 342

PVS-Studio warning: V560 A part of conditional expression is always true. KoTextLayoutArea.cpp 1622

qreal KoTextLayoutArea::addLine(QTextLine &line,
                                FrameIterator *cursor,
                                KoTextBlockData &blockData)
{
  if (!d->documentLayout->changeTracker()
   || !d->documentLayout->changeTracker()->displayChanges() // <=
   || !d->documentLayout->changeTracker()->...
   || !d->documentLayout->changeTracker()->...
   || !d->documentLayout->changeTracker()->elementById(....)
   || !d->documentLayout->changeTracker()->elementById(....)
   || ....
   || d->documentLayout->changeTracker()->displayChanges()) { // <=
     ....
  }
}

If you look closely, you will notice that within this complex condition there is a check of the type (!a || a):

d->documentLayout->changeTracker()->displayChanges() ||
!d->documentLayout->changeTracker()->displayChanges()

This condition is always true, that is why this whole big check becomes meaningless, as reported by the analyzer.

PVS-Studio warning: V547 Expression 'n == 128' is always false. compression.cpp 110

PVS-Studio warning: V547 Expression 'n > 128' is always false. compression.cpp 112

quint32 decode_packbits(const char *src,
                        char* dst,
                        quint16 packed_len,
                        quint32 unpacked_len)
{
    qint32    n;
    ....
    while (unpack_left > 0 && pack_left > 0)
    {
        n = *src;
        src++;
        pack_left--;

        if (n == 128) // <=
            continue;
        else if (n > 128) // <=
            n -= 256;
        ....
    }
    ....
}

In this example, a value of the type const char, obtained by dereferencing the src pointer, is written in the variable n of the type qint32, that is why the range of the values for a variable n is as follows: [-128; 127]. Then the variable n is compared with the number 128, though it is clear that the result of such a check is always false.

Note: Project is built without -funsigned-char.

PVS-Studio warning: V590 Consider inspecting the 'state == (- 3) || state != 0' expression. The expression is excessive or contains a misprint. psd_pixel_utils.cpp 335

psd_status 
psd_unzip_without_prediction(psd_uchar *src_buf, psd_int src_len,
                             psd_uchar *dst_buf, psd_int dst_len)
{
    do {
        state = inflate(&stream, Z_PARTIAL_FLUSH);
        if(state == Z_STREAM_END)
            break;
        if(state == Z_DATA_ERROR || state != Z_OK) // <=
            break;
    }  while (stream.avail_out > 0);
}

Here the developers overthought with the second condition, that's why it became redundant. I suppose that the correct version looks as follows:

    do {
        state = inflate(&stream, Z_PARTIAL_FLUSH);
        if(state != Z_OK)
            break;
    }  while (stream.avail_out > 0);

PVS-Studio warning: V547 Expression is always false. SvgTextEditor.cpp 472

void SvgTextEditor::setTextWeightDemi()
{
    if (m_textEditorWidget.richTextEdit->textCursor()
          .charFormat().fontWeight() > QFont::Normal
        && m_textEditorWidget.richTextEdit->textCursor()
           .charFormat().fontWeight() < QFont::Normal) { // <=
        setTextBold(QFont::Normal);
    } else {
        setTextBold(QFont::DemiBold);
    }
}

The analyzer has detected the condition (a > b && a < b), which is, obviously, always false. It's difficult to say what exactly the author wanted to write, but this code is clearly erroneous and needs correction.

Typos

PVS-Studio warning: V547 Expression is always true. Probably the '&&' operator should be used here. KoResourceItemChooser.cpp 408

void KoResourceItemChooser::updatePreview(KoResource *resource)
{
    ....
    if (image.format() != QImage::Format_RGB32 || // <=
    image.format() != QImage::Format_ARGB32 ||    // <=
    image.format() != QImage::Format_ARGB32_Premultiplied) {

        image = image.convertToFormat(....);
    }
    ....
}

A programmer made a mistake and instead of writing &&, wrote operator ||, leaving all its condition meaningless, because in the result he got the condition: a != const_1 || a != const_2, which is always true.

PVS-Studio warning: V547 Expression is always true. Probably the '&&' operator should be used here. KoSvgTextShapeMarkupConverter.cpp 1000

QString KoSvgTextShapeMarkupConverter::style(....)
{
  ....
  if (format.underlineStyle() != QTextCharFormat::NoUnderline ||
      format.underlineStyle() != QTextCharFormat::SpellCheckUnderline)
  {
      ....
  }
  ....
}

The case, similar to the previous one: instead of the && operator wrote ||.

PVS-Studio warning: V501 There are identical sub-expressions 'sensor(FUZZY_PER_DAB, true)' to the left and to the right of the '||' operator. kis_pressure_size_option.cpp 43

void KisPressureSizeOption::lodLimitations(....) const
{
  if (sensor(FUZZY_PER_DAB, true) || sensor(FUZZY_PER_DAB, true)) {
      l->limitations << KoID("size-fade", i18nc("...."));
  }

  if (sensor(FADE, true)) {
      l->blockers << KoID("...."));
  }
}

The analyzer has detected a situation where on the left and on the right of the operator || there are same expressions. If you look at the DynamicSensorType enumeration:

enum DynamicSensorType {
    FUZZY_PER_DAB,
    FUZZY_PER_STROKE,
    SPEED,
    FADE,
    ....
    UNKNOWN = 255
};

it becomes clear that most likely a developer wanted to write FUZZY_PER_STROKE on the right, rather than FUZZY_PER_DAB.

Static analyzers are great at detecting such errors, whereas during code-review it is easy to overlook them, especially when you have to view a big number of changes.

Errors Caused by Copy-Paste

Picture 6

PVS-Studio warning: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: d->paragraphStylesDotXmlStyles.values(). KoTextSharedLoadingData.cpp 594

QList<KoParagraphStyle *> 
KoTextSharedLoadingData::paragraphStyles(bool stylesDotXml) const
{
    return stylesDotXml ? 
        d->paragraphStylesDotXmlStyles.values() :
        d->paragraphStylesDotXmlStyles.values(); // <=
}

Most likely, a programmer copied the block then in the ternary operator and forgot to change the name of the called method, because of which regardless of the condition, one value will always be returned.

Judging by the previous method:

KoParagraphStyle *
KoTextSharedLoadingData::paragraphStyle(const QString &name,
                                        bool stylesDotXml) const
{
    return stylesDotXml ? 
        d->paragraphStylesDotXmlStyles.value(name) :
        d->paragraphContentDotXmlStyles.value(name);
}

in the else block one has to write paragraphContentDotXmlStyles instead of paragraphStylesDotXmlStyles.

PVS-Studio warning: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: qFloor(axis). kis_transform_worker.cc 456

void mirror_impl(KisPaintDeviceSP dev, qreal axis, bool isHorizontal)
{
    ....
    int leftCenterPoint = qFloor(axis) < axis ? qFloor(axis) :
                                                qFloor(axis); // <=
    int leftEnd = qMin(leftCenterPoint, rightEnd);

    int rightCenterPoint = qFloor(axis) < axis ? qCeil(axis) :
                                                 qFloor(axis);
    int rightStart = qMax(rightCenterPoint, leftStart);
    ....
}

Another triggering, very similar to the previous one. Perhaps, in the then block of the first ternary operator a developer wanted to write qCeil(axis), not qFloor(axis), or the condition here is even redundant.

PVS-Studio warning: V656 Variables 'vx', 'vy' are initialized through the call to the same function. It's probably an error or un-optimized code. Check lines: 218, 219. KarbonSimplifyPath.cpp 219

bool KarbonSimplifyPath::isSufficentlyFlat(QPointF curve[4])
{
  qreal ux = 3 * curve[1].x() - 2 * curve[0].x() - curve[3].x();
  qreal uy = 3 * curve[1].y() - 2 * curve[0].y() - curve[3].y();
  qreal vx = 3 * curve[2].x() - 2 * curve[3].x() - curve[0].x(); // <=
  qreal vy = 3 * curve[2].x() - 2 * curve[3].x() - curve[0].x(); // <=
  ....
}

This code looks very suspicious, as, most likely, when writing formula for vy, one copied the previous line, but forgot to change the x() calls to y(). In case if there is no error here, it's better to rewrite the code as follows:

qreal vx = 3 * curve[2].x() - 2 * curve[3].x() - curve[0].x();
qreal vy = vx;

PVS-Studio warning: V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 675, 679. KoTableCellStyle.cpp 679

void KoTableCellStyle::loadOdfProperties(
    KoShapeLoadingContext &context,
    KoStyleStack &styleStack)
{
  ....
  if (styleStack.hasProperty(KoXmlNS::style, "print-content"))
  {
    setPrintContent(styleStack.property(KoXmlNS::style,
                      "print-content") == "true");
  }

  if (styleStack.hasProperty(KoXmlNS::style, "repeat-content")) // <=
  {
    setRepeatContent(styleStack.property(KoXmlNS::style,
                       "repeat-content") == "true");
  }

  if (styleStack.hasProperty(KoXmlNS::style, "repeat-content")) // <=
  {
    setRepeatContent(styleStack.property(KoXmlNS::style,
                       "repeat-content") == "true");
  }
  ....
}

The same check is performed twice. In this method, a developer either copied something extra, or mixed up something. If there is no error, then one has to remove the repeated code.

PVS-Studio warning: V523 The 'then' statement is equivalent to the 'else' statement. kis_processing_applicator.cpp 227

void KisProcessingApplicator::applyVisitorAllFrames(....)
{
    KisLayerUtils::FrameJobs jobs;

    if (m_flags.testFlag(RECURSIVE)) {
        KisLayerUtils::updateFrameJobsRecursive(&jobs, m_node); // <=
    } else {
        KisLayerUtils::updateFrameJobsRecursive(&jobs, m_node); // <=
    }
    
    ....
}

Probably, one copied the code from the block then to the block else and forgot to change the called method. Judging from the project code, a developer probably wanted to write KisLayerUtils::updateFrameJobs in the else branch.

Repeated condition (error in condition)

PVS-Studio warning: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 255, 269. KoInlineTextObjectManager.cpp 255

void 
KoInlineTextObjectManager::documentInformationUpdated(
const QString &info, const QString &data)
{
    if (info == "title") // <=
        setProperty(KoInlineObject::Title, data);
    else if (info == "description")
        setProperty(KoInlineObject::Description, data);
    else if (info == "abstract")
        setProperty(KoInlineObject::Comments, data);
    else if (info == "subject")
        setProperty(KoInlineObject::Subject, data);
    else if (info == "keyword")
        setProperty(KoInlineObject::Keywords, data);
    else if (info == "creator")
        setProperty(KoInlineObject::AuthorName, data);
    else if (info == "initial")
        setProperty(KoInlineObject::AuthorInitials, data);
    else if (info == "title") // <=
        setProperty(KoInlineObject::SenderTitle, data);
    else if (info == "email")
        setProperty(KoInlineObject::SenderEmail, data);
    ....
}

Here one check is performed twice. Most likely, in the second case, one had to write something like "sender-title".

Errors occurring when working with enumeration constants

PVS-Studio warning: V768 The enumeration constant 'BatchMode' is used as a variable of a Boolean-type. KisMainWindow.cpp 811

bool KisMainWindow::openDocument(const QUrl &url, OpenFlags flags)
{
    if (!QFile(url.toLocalFile()).exists()) {
        if (!flags && BatchMode) {              // <=
            QMessageBox::critical(0,
                                  i18nc("....", "Krita"),
                                  i18n("....", url.url()));
        }
        ....
    }
    ....
}

BatchMode is the member of enumeration OpenFlag with the value of 0x2:

enum OpenFlag {
    None = 0,
    Import = 0x1,
    BatchMode = 0x2,
    RecoveryFile = 0x4
};

In this example, however, it is handled as if it is a variable. In the result, it turns out that a part of the condition is always true.

While in the above code, BatchMode is handled correctly:

if (flags & BatchMode) {
    newdoc->setFileBatchMode(true);
}

From this we can conclude that developers wanted to write something like this:

bool KisMainWindow::openDocument(const QUrl &url, OpenFlags flags)
{
    if (!QFile(url.toLocalFile()).exists()) {
        if (!(flags & BatchMode)) {            // <=
            QMessageBox::critical(0,
                                  i18nc("....", "Krita"),
                                  i18n("....", url.url()));
        }
        ....
    }
    ....
}

PVS-Studio warning: V768 The enumeration constant 'State_Active' is used as a variable of a Boolean-type. KisOpenPane.cpp 104

void paint(....) const override
{
    QStyledItemDelegate::paint(painter, option, index);

    if(!(option.state & (int)(QStyle::State_Active &&  // <=
                              QStyle::State_Enabled))) // <=
    {
        ....
    }
}

In this case, apparently, the authors of the code mixed up the operators and instead of | inside the mask used the operator &&. I think, the corrected version should be as follows:

void paint(....) const override
{
    QStyledItemDelegate::paint(painter, option, index);

    if(!(option.state & (int)(QStyle::State_Active |
                              QStyle::State_Enabled)))
    {
        ....
    }
}

Suspicious Repeated Assignments

PVS-Studio warning: V519 The 'value' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 61, 66. kis_draggable_tool_button.cpp 66

int KisDraggableToolButton::continueDrag(const QPoint &pos)
{
    ....

    if (m_orientation == Qt::Horizontal) {
        value = diff.x(); // <=
    } else {
        value = -diff.y(); // <=
    }

    value = diff.x() - diff.y(); // <=

    return value;
}

The variable value is assigned a value within the condition, but then immediately the value is overwritten. Most likely, there's some sort of an error.

PVS-Studio warning: V519 The 'uf.f' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 263, 265. lut.h 265

PVS-Studio warning: V519 The 'uf.f' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 271, 273. lut.h 273

LutKey<float>(float min, float max, float precision) :
    m_min(min), m_max(max), m_precision(precision)
{
    ....
    if(m_min > 0 && m_max > 0)
    {
        uf.f = m_min;                // <=
        m_tMin_p = uf.i >> m_shift;
        uf.f = m_max;                // <=
        m_tMax_p = uf.i >> m_shift;
        m_tMin_n = m_tMax_p;
        m_tMax_n = m_tMax_p;
    } 
    else if( m_max < 0)
    {
        uf.f = m_min;                // <=
        m_tMax_n = uf.i >> m_shift;
        uf.f = m_max;                // <=
        m_tMin_n = uf.i >> m_shift;
        m_tMin_p = m_tMax_n;
        m_tMax_p = m_tMax_n;
    }
    ....
}

The variable uf.f is assigned with different values twice. It is suspicious and it is quite possible that developers wanted to assign a value to another variable.

Perhaps Else Is Omitted Here

PVS-Studio warning: V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. SvgStyleWriter.cpp 82

void SvgStyleWriter::saveSvgBasicStyle(KoShape *shape,
                                       SvgSavingContext &context)
{
    if (!shape->isVisible(false)) {
        ....
    } if (shape->transparency() > 0.0) { // <=
        ....
    }
}

Here, perhaps, one forgot keyword else. Even if there is no error, code formatting is worth fixing not to confuse the analyzer and other programmers.

A similar warning:

  • V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. transform_stroke_strategy.cpp 166

Problems with Null Pointers

Picture 3

PVS-Studio warning: V595 The 'l' pointer was utilized before it was verified against nullptr. Check lines: 428, 429. kis_node_manager.cpp 428

void KisNodeManager::moveNodeAt(....)
{
    ....
    KisLayer *l = qobject_cast<KisLayer*>(parent.data());
    KisSelectionMaskSP selMask = l->selectionMask(); // <=
    if (m && m->active() && l && l->selectionMask()) // <=
    selMask->setActive(false);
    ....
}

Here the pointer l is first dereferenced and only after that checked for nullptr.

Similar analyzer warnings:

  • V595 The 'gradient' pointer was utilized before it was verified against nullptr. Check lines: 164, 166. kis_gradient_chooser.cc 164
  • V595 The 'm_currentShape' pointer was utilized before it was verified against nullptr. Check lines: 316, 325. ArtisticTextTool.cpp 316
  • V595 The 'painter()' pointer was utilized before it was verified against nullptr. Check lines: 87, 89. kis_grid_paintop.cpp 87
  • V595 The 'm_optionsWidget' pointer was utilized before it was verified against nullptr. Check lines: 193, 202. kis_tool_move.cc 193
  • ....

PVS-Studio warning: V1004 The 'sb' pointer was used unsafely after it was verified against nullptr. Check lines: 665, 670. KisView.cpp 670

void KisView::slotSavingStatusMessage(const QString &text,
                                      int timeout,
                                      bool isAutoSaving)
{
    QStatusBar *sb = statusBar();
    if (sb) // <=
        sb->showMessage(text, timeout);

    KisConfig cfg;

    if (sb->isHidden() || // <=
        (!isAutoSaving && cfg.forceShowSaveMessages()) ||
        (cfg.forceShowAutosaveMessages() && isAutoSaving)) {

        viewManager()->showFloatingMessage(text, QIcon());
    }
}

The analyzer considers unsafe such a usage of a pointer sb after its checking for nullptr. Indeed, if the pointer is null (which is allowed, once such a condition is written above), then when calling sb->isHidden(), null pointer dereference might occur. As a hotfix, one can add a check for nullptr in the second condition as well, or handle this situation differently.

The similar analyzer warning:

  • V1004 The 'd->viewManager' pointer was used unsafely after it was verified against nullptr. Check lines: 338, 365. KisView.cpp 365

PVS-Studio warning: V522 Dereferencing of the null pointer 'slot' might take place. kis_spriter_export.cpp 568

KisImportExportFilter::ConversionStatus KisSpriterExport::convert(
    KisDocument *document,
    QIODevice *io, 
    KisPropertiesConfigurationSP /*configuration*/)
{
    ....
    SpriterSlot *slot = 0;                                   // <=

    // layer.name format: "base_name bone(bone_name) slot(slot_name)"
    if (file.layerName.contains("slot(")) {
        int start = file.layerName.indexOf("slot(") + 5;
        int end = file.layerName.indexOf(')', start);
        slot->name = file.layerName.mid(start, end - start); // <=
        slot->defaultAttachmentFlag = ....                   // <=
    }
    ....
}

In this example, a dereference of the null pointer slot will certainly occur, which in turn results in undefined behavior.

Memory Leaks

PVS-Studio warning: V773 The function was exited without releasing the 'svgSymbol' pointer. A memory leak is possible. SvgParser.cpp 681

bool SvgParser::parseSymbol(const KoXmlElement &e)
{
    ....

    KoSvgSymbol *svgSymbol = new KoSvgSymbol();         // <=

    // ensure that the clip path is loaded in local coordinates system
    m_context.pushGraphicsContext(e, false);
    m_context.currentGC()->matrix = QTransform();
    m_context.currentGC()->currentBoundingBox = QRectF(0.0, 0.0,
                                                       1.0, 1.0);

    QString title = e.firstChildElement("title").toElement().text();

    KoShape *symbolShape = parseGroup(e);

    m_context.popGraphicsContext();

    if (!symbolShape) return false;                     // <=
    ....
}

In this example, when exiting the method one forgot to release the memory that has been allocated for svgSymbol. This is a memory leak. The project has many such leaks, but they are quite similar, so I won't discuss them a lot.

Similar analyzer warnings:

  • V773 The function was exited without releasing the 'ppmFlow' pointer. A memory leak is possible. kis_ppm_import.cpp 249
  • V773 The function was exited without releasing the 'keyShortcut' pointer. A memory leak is possible. kis_input_manager_p.cpp 443
  • V773 The function was exited without releasing the 'layerRecord' pointer. A memory leak is possible. psd_layer_section.cpp 109
  • V773 The function was exited without releasing the 'filterStack' pointer. A memory leak is possible. FilterEffectResource.cpp 139
  • ....

Checks for Nullptr After New

PVS-Studio warning: V668 There is no sense in testing the 'charStyle' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. CharacterGeneral.cpp 153

bool KoPathShape::separate(QList<KoPathShape*> & separatedPaths)
{
    ....

    Q_FOREACH (KoSubpath* subpath, d->subpaths) {
        KoPathShape *shape = new KoPathShape();
        if (! shape) continue; // <=
        ....
    }
}

It seems to me that this topic has become regular in our articles. It is pointless to check a pointer for nullptr if the memory was allocated by the operator new. If it impossible to allocate memory, the new operator throws an exception std::bad_alloc(), it doesn't return nullptr. To fix this code, you can add an exception handler, or use new (std: nothrow).

Similar analyzer warnings:

  • V668 There is no sense in testing the 'factory' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. TestKoShapeFactory.cpp 36
  • V668 There is no sense in testing the 'parStyle' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. ParagraphGeneral.cpp 199
  • V668 There is no sense in testing the 'spline' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. multi_bspline_create.cpp 460
  • V668 There is no sense in testing the 'm_currentStrategy' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. ConnectionTool.cpp 333
  • ....

Refactoring

PVS-Studio warning: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!nodeJuggler' and 'nodeJuggler'. kis_node_manager.cpp 809

if (!nodeJuggler ||                           // <=
    (nodeJuggler &&                           // <=
     (nodeJuggler->isEnded() ||
      !nodeJuggler->canMergeAction(actionName)))) {
        ....
}

This check can be simplified:

if (!nodeJuggler ||
    (nodeJuggler->isEnded() ||
     !nodeJuggler->canMergeAction(actionName))) {
        ....
}

Similar analyzer warning:

  • V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!m_currentFilterConfigWidget' and 'm_currentFilterConfigWidget'. kis_filter_option.cpp 111

PVS-Studio warning: V501 There are identical sub-expressions to the left and to the right of the '&&' operator: !iterator.atEnd() &&!iterator.atEnd() KoTextDebug.cpp 867

void KoTextDebug::dumpFrame(const QTextFrame *frame, QTextStream &out)
{
    ....
    
    QTextFrame::iterator iterator = frame->begin();

    for (; !iterator.atEnd() && !iterator.atEnd(); ++iterator) { // <=
        ....
    }
    
    ....
}

It is worth checking the condition of the loop for errors. If there are no errors, it is needed to remove a repeated check.

The similar analyzer warning:

  • V501 There are identical sub-expressions to the left and to the right of the '&&' operator: !iterator.atEnd() &&!iterator.atEnd() KoTextDebug.cpp 909

PVS-Studio warning: V799 The 'cmd' variable is not used after memory has been allocated for it. Consider checking the use of this variable. kis_all_filter_test.cpp 154

bool testFilter(KisFilterSP f)
{
  ....
  KisTransaction * cmd = 
    new KisTransaction(kundo2_noi18n(f->name()), dev); // <=

  // Get the predefined configuration from a file
  KisFilterConfigurationSP  kfc = f->defaultConfiguration();

  QFile file(QString(FILES_DATA_DIR) +
             QDir::separator() + f->id() + ".cfg");
  if (!file.open(QIODevice::ReadOnly | QIODevice::Text)) {
    //dbgKrita << "creating new file for " << f->id();
    file.open(QIODevice::WriteOnly | QIODevice::Text);
    QTextStream out(&file);
    out.setCodec("UTF-8");
    out << kfc->toXML();
  } else {
    QString s;
    QTextStream in(&file);
    in.setCodec("UTF-8");
    s = in.readAll();
    //dbgKrita << "Read for " << f->id() << "\n" << s;
    kfc->fromXML(s);
  }
  dbgKrita << f->id();// << "\n" << kfc->toXML() << "\n";

  f->process(dev, QRect(QPoint(0,0), qimage.size()), kfc);

  QPoint errpoint;

  delete cmd; // <=

  ....
}

Here the memory was allocated and released for the object cmd, but it wasn't used even once.

PVS-Studio warning: V732 Unary minus operator does not modify a bool type value. Consider using the '!' operator. kis_equalizer_slider.cpp 75

QRect KisEqualizerSlider::Private::boundingRect() const
{
    QRect bounds = q->rect().adjusted(0, 0, -isRightmost, -1);
    return bounds;
}

In this example, the variable isRightmost is of the type bool. Using the unary minus, the variable is implicitly converted into the type int and the resulting number is passed in the method adjusted(). Such code makes the understanding more difficult. Explicit is better than implicit, so I think I'd rewrite this fragment like this:

QRect KisEqualizerSlider::Private::boundingRect() const
{
    QRect bounds = q->rect().adjusted(0, 0, isRightmost ? -1 : 0, -1);
    return bounds;
}

Similar analyzer warnings:

  • V732 Unary minus operator does not modify a bool type value. Consider using the '!' operator. kis_equalizer_button.cpp 66
  • V732 Unary minus operator does not modify a bool type value. Consider using the '!' operator. kis_duplicateop.cpp 222

Conclusion

In conclusion, I would like to appeal to the developers of Krita and offer them to resume free using of our analyzer.

Since the last time when the developers used PVS-Studio, we have released the versions for Linux and MacOS (I tested this project in Linux myself), and the analysis was significantly improved.

Besides, I suggest downloading and trying out PVS-Studio on the code of your project.



Use PVS-Studio to search for bugs in C, C++, and C# code

We offer you to check your project code with PVS-Studio. Just one bug found in the project will show you the benefits of the static code analysis methodology better than a dozen of the articles.

goto PVS-Studio;

Egor Bredikhin
Articles: 6


Do you make errors in the code?

Check your code
with PVS-Studio

Static code analysis
for C, C++, and C#

goto PVS-Studio;