Checking the source code of the free graphical editor Krita 4.0

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

Introduction

It is noteworthy that the developers already used PVS-Studio back in 2015 on the version of Krita 2.9.2 and successfully fixed errors with it . However, apparently, the analyzer has not been used since. In all our articles, we say that regular checks are important, because if the developers continued to use PVS-Studio, then the errors that I will discuss in this article would simply not be included in the release.

Useless range-based for

PVS-Studio Warning: V714 Variable row is not passed into foreach 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, the programmer obviously wanted to multiply each element of the rows container by invM33 , however, this will not happen. At each iteration of the loop, a new variable is created with the name row , which is then simply destroyed. To fix the error, you need to create a link to the item stored in the 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 enter a block under the condition j == 0 , since this condition is always false due to the fact that the restriction j> 0 is imposed in the for loop above . 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 inside this difficult condition is a check of the form (! A || a) :

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

Such a condition is always true, because of which all this big check becomes meaningless, as the analyzer reports.

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 type const char obtained by dereferencing the src pointer is written to the variable n of type qint32 , therefore the range of valid values ​​of the variable n is : [-128; 127]. Then the variable n is compared with the number 128, although it is clear that the result of such a check is always false . Note: The 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 they were too smart with the second condition, because of which it became redundant. I will assume that the correct option is 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 detected a condition of the form (a> b && a <b) , which, obviously, is always false. I am at a loss to say what exactly the author wanted to write, however, this code is definitely erroneous and needs to be fixed.

Eyelids

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(....);
    }
    ....
}

The programmer made a mistake and wrote the || operator instead of the && operator , because of which all of his condition became meaningless, because it turned out a condition of the form: 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 is similar to the previous one: they mixed up the logical operator and instead of && 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 detected a situation where the left and right of the operator || same expressions are found. If you take a look at the DynamicSensorType enumeration :

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

it becomes clear that on the right, most likely, they wanted to write FUZZY_PER_STROKE , and not FUZZY_PER_DAB .

Static analyzers do a good job of finding such errors, but they are easy to overlook code reviews, especially when you need to view a large number of changes.

Copy-Paste Errors

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 
KoTextSharedLoadingData::paragraphStyles(bool stylesDotXml) const
{
    return stylesDotXml ? 
        d->paragraphStylesDotXmlStyles.values() :
        d->paragraphStylesDotXmlStyles.values(); // <=
}

Here, most likely, they then copied the then block in the ternary operator and forgot to change the name of the method being called, which is why, regardless of the condition, a single 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 , write paragraphContentDotXmlStyles , instead of paragraphStylesDotXmlStyles .

Warning PVS-Studio: 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 operation, very similar to the previous one. Perhaps in the then block of the first ternary operator they wanted to write qCeil (axis) , not qFloor (axis) , or the condition is generally superfluous here.

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 rather suspicious, since most likely when writing the formula for vy we copied the previous line, but forgot to change the calls of x () to y () . If there is still no error here, it is better to rewrite the code like this:

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 located 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, they either copied the excess, or mixed something up. If there is no error, then it is worth removing the duplicate 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); // <=
    }
    ....
}

Most likely, here they copied the code from the then block to the else block and forgot to change the called method. Judging by the project code, in the else branch, they probably wanted to write KisLayerUtils :: updateFrameJobs .

Duplicate 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 the same check is performed twice. Most likely, in the second case it was necessary to write something like “sender-title” .

Errors 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 an OpenFlag enumeration element with a value of 0x2 :

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

However, in this example, they work with it as if it were a variable. The result is that part of the condition is always true.

At the same time, in the code above, they work correctly with BatchMode :

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

From this we can conclude that they 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 operators were confused and instead of | inside the mask, the && operator was used . I think that the corrected version should look like this:

void paint(....) const override
{
    QStyledItemDelegate::paint(painter, option, index);
    if(!(option.state & (int)(QStyle::State_Active |
                              QStyle::State_Enabled)))
    {
        ....
    }
}

Suspicious Reassignments

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 value variable is assigned some value inside the condition, however, then this value is immediately overwritten. Most likely, there is some kind of mistake.

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
Warning PVS-Studio: V519 The 'uf.f' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 271, 273. lut.h 273

LutKey(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 different values ​​two times in a row. This is suspicious, and it is possible that they wanted to assign a value to some other variable.

Possibly skipped else

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, you may have forgotten the else keyword . Even if there is no error here, it is worth adjusting the code formatting so as 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

Null Pointer Issues

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(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 then 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 the use of the sb pointer after checking it for nullptr . And indeed, if the pointer is null (and this is allowed, since such a condition is written above), then when sb-> isHidden () is called, dereference of the null pointer may occur. As a correction, you can add a check for nullptr to the second condition, or somehow handle this situation.

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, the dereferencing of the slot null pointer is guaranteed to occur , which in turn leads to undefined program 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, they forgot to free the memory allocated for svgSymbol . This is a memory leak. The project has many similar leaks, but they are of the same type, so I will not dwell on them.

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 & separatedPaths)
{
    ....
    Q_FOREACH (KoSubpath* subpath, d->subpaths) {
        KoPathShape *shape = new KoPathShape();
        if (! shape) continue; // <=
        ....
    }
}

This section in our articles seems to have become permanent. It is pointless to check for a pointer to nullptr if memory has been allocated using the new operator . If it is not possible to allocate memory, the new operator throws an exception std :: bad_alloc () , and does not 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
• ....

Рефакторинг

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)))) {
        ....
}

Such verification 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

Warning PVS-Studio: 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 for loop for errors. If there are no errors, you should remove the duplicate check.

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, they allocated and freed memory for the cmd object , but they never used it.

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 isRightmost variable is of type bool . Using the unary minus, the variable is implicitly converted to type int and passed the resulting number to the adjusted () method . Such code complicates understanding. Explicit is better than implicit, so I think you should rewrite this snippet 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 contact the Krita developers and offer them to resume using our analyzer for free .

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

In addition, I invite everyone to download and try PVS-Studio on the code of their project.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Egor Bredikhin. Check of the Krita 4.0 Free Source Code Graphics Editor