Введение
Примечательно, что разработчики уже использовали PVS-Studio в далеком 2015 году на версии Krita 2.9.2 и успешно исправляли с его помощью ошибки. Однако, видимо, с тех пор анализатор не использовали. Во всех наших статьях мы говорим о том, что важны именно регулярные проверки, ведь если бы разработчики продолжали пользоваться PVS-Studio, то ошибки, о которых я расскажу в этой статье, попросту не попали бы в релиз.
Бесполезный range-based for
Предупреждение PVS-Studio: 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;
}
}
....
}
В данном примере программист, очевидно, хотел умножить каждый элемент контейнера rows на invM33, однако, этого не произойдёт. На каждой итерации цикла создаётся новая переменная с именем row, которая затем просто уничтожается. Чтобы исправить ошибку необходимо создавать ссылку на элемент, хранящийся в контейнере:
for (auto &row : rows) {
row *= invM33;
}
Некорректные условия
Предупреждение PVS-Studio: 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] = ....
}
}
}
....
}
Программа никогда не зайдет в блок под условием j==0, поскольку это условие всегда ложно из-за того, что выше в цикле for накладывается ограничение j > 0.
Аналогичные предупреждения анализатора:
- 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: 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()) { // <=
....
}
}
Если присмотреться, то можно заметить, что внутри данного сложного условия находится проверка вида (!a || a):
d->documentLayout->changeTracker()->displayChanges() ||
!d->documentLayout->changeTracker()->displayChanges()
Такое условие является всегда истинным, из-за чего вся эта большая проверка становится бессмысленной, о чем и сообщает анализатор.
Предупреждение PVS-Studio: V547 Expression 'n == 128' is always false. compression.cpp 110
Предупреждение PVS-Studio: 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;
....
}
....
}
В данном примере в переменную n типа qint32 записывается значение типа const char, получаемое при разыменовании указателя src, поэтому диапазон допустимых значений переменной n: [-128; 127]. Затем переменная n сравнивается с числом 128, хотя понятно, что результат такой проверки всегда false.
Примечание: Проект собирается без -funsigned-char.
Предупреждение PVS-Studio: 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);
}
Здесь перемудрили со вторым условием, из-за чего оно стало избыточным. Предположу, что корректный вариант выглядит следующим образом:
do {
state = inflate(&stream, Z_PARTIAL_FLUSH);
if(state != Z_OK)
break;
} while (stream.avail_out > 0);
Предупреждение PVS-Studio: 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);
}
}
Анализатор обнаружил условие вида (a > b && a < b), которое, очевидно, является всегда ложным. Затрудняюсь сказать, что именно хотел написать автор, однако, данный код однозначно является ошибочным и нуждается в исправлении.
Очепятки
Предупреждение PVS-Studio: 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 != const_1 || a != const_2, которое является всегда истинным.
Предупреждение PVS-Studio: 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)
{
....
}
....
}
Случай аналогичный предыдущему: перепутали логический оператор и вместо && написали ||.
Предупреждение PVS-Studio: 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("...."));
}
}
Анализатор обнаружил ситуацию, когда слева и справа от оператора || находятся одинаковые выражения. Если взглянуть на перечисление DynamicSensorType:
enum DynamicSensorType {
FUZZY_PER_DAB,
FUZZY_PER_STROKE,
SPEED,
FADE,
....
UNKNOWN = 255
};
то становится ясно, что справа, скорее всего, хотели написать FUZZY_PER_STROKE, а не FUZZY_PER_DAB.
Статические анализаторы хорошо справляются с поиском подобных ошибок, а вот на код-ревью их легко проглядеть, особенно, когда нужно просмотреть большое количество изменений.
Ошибки из-за Copy-Paste
Предупреждение PVS-Studio: 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(); // <=
}
Здесь, скорее всего, скопировали блок then в тернарном операторе и забыли изменить имя вызываемого метода, из-за чего, в независимости от условия, всегда будет возвращаться одно значение.
Судя по предыдущему методу:
KoParagraphStyle *
KoTextSharedLoadingData::paragraphStyle(const QString &name,
bool stylesDotXml) const
{
return stylesDotXml ?
d->paragraphStylesDotXmlStyles.value(name) :
d->paragraphContentDotXmlStyles.value(name);
}
в блоке else нужно написать paragraphContentDotXmlStyles, вместо paragraphStylesDotXmlStyles.
Предупреждение 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);
....
}
Еще одно срабатывание, очень похожее на предыдущее. Возможно, в блоке then первого тернарного оператора хотели написать qCeil(axis), а не qFloor(axis), либо же условие здесь вообще лишнее.
Предупреждение PVS-Studio: 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(); // <=
....
}
Данный код выглядит весьма подозрительно, так как, скорее всего, при написании формулы для vy скопировали предыдущую строку, но забыли поменять вызовы x() на y(). В случае, если ошибки здесь все-таки нет, лучше переписать код так:
qreal vx = 3 * curve[2].x() - 2 * curve[3].x() - curve[0].x();
qreal vy = vx;
Предупреждение PVS-Studio: 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");
}
....
}
Два раза выполняется одна и та же проверка. В этом методе либо скопировали лишнего, либо что-то перепутали. Если ошибки здесь нет, то стоит удалить дублирующийся код.
Предупреждение PVS-Studio: 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); // <=
}
....
}
Скорее всего, здесь скопировали код из блока then в блок else и забыли поменять вызываемый метод. Судя по коду проекта, в ветке else наверняка хотели написать KisLayerUtils::updateFrameJobs.
Дублирующееся условие (ошибка в условии)
Предупреждение PVS-Studio: 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);
....
}
Здесь два раза выполняется одна и та же проверка. Скорее всего, во втором случае нужно было написать что-то вроде «sender-title».
Ошибки при работе с константами из перечисления
Предупреждение PVS-Studio: 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 — это элемент перечисления OpenFlag со значением 0x2:
enum OpenFlag {
None = 0,
Import = 0x1,
BatchMode = 0x2,
RecoveryFile = 0x4
};
Однако в данном примере с ним работают, как будто это переменная. В результате получается, что часть условия всегда истинна.
При этом в коде выше с BatchMode работают корректно:
if (flags & BatchMode) {
newdoc->setFileBatchMode(true);
}
Из этого можно сделать вывод, что хотели написать нечто подобное:
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: 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))) // <=
{
....
}
}
В данном случае, судя по всему, перепутали операторы и вместо | внутри маски использовали оператор &&. Думаю, что исправленный вариант должен выглядеть следующим образом:
void paint(....) const override
{
QStyledItemDelegate::paint(painter, option, index);
if(!(option.state & (int)(QStyle::State_Active |
QStyle::State_Enabled)))
{
....
}
}
Подозрительные повторные присваивания
Предупреждение PVS-Studio: 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;
}
Переменной value присваивают некоторое значение внутри условия, однако, затем сразу же перезаписывают это значение. Скорее всего, здесь есть какая-то ошибка.
Предупреждение PVS-Studio: V519 The 'uf.f' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 263, 265. lut.h 265
Предупреждение 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>(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;
}
....
}
Переменной uf.f два раза подряд присваиваются разные значения. Это подозрительно, и вполне возможно, что хотели присвоить значение какой-то другой переменной.
Возможно, пропущен else
Предупреждение PVS-Studio: 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) { // <=
....
}
}
Здесь, возможно, забыли ключевое слово else. Даже если ошибки здесь нет, стоит поправить форматирование кода, чтобы не смущать анализатор и других программистов.
Аналогичное предупреждение:
- V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. transform_stroke_strategy.cpp 166
Проблемы с нулевыми указателями
Предупреждение PVS-Studio: 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);
....
}
Здесь указатель l сначала разыменовывают и только потом проверяют его на nullptr.
Аналогичные предупреждения анализатора:
- 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: 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());
}
}
Анализатор считает небезопасным подобное использование указателя sb после проверки его на nullptr. И действительно, в случае, если указатель будет нулевым (а это допускается, раз такое условие написано выше), то при вызове sb->isHidden() может произойти разыменование нулевого указателя. В качестве исправления можно добавить проверку на nullptr и во второе условие, либо еще как-то обработать эту ситуацию.
Аналогичное предупреждение анализатора:
- V1004 The 'd->viewManager' pointer was used unsafely after it was verified against nullptr. Check lines: 338, 365. KisView.cpp 365
Предупреждение PVS-Studio: 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 = .... // <=
}
....
}
В данном примере гарантированно произойдет разыменование нулевого указателя slot, что в свою очередь приводит к неопределенному поведению программы.
Утечки памяти
Предупреждение PVS-Studio: 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; // <=
....
}
В этом примере при выходе из метода забыли освободить память, выделенную под svgSymbol. Это утечка памяти. В проекте много подобных утечек, но они достаточно однотипны, так что останавливаться на них я не буду.
Аналогичные предупреждения анализатора:
- 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
- ....
Проверки на nullptr после new
Предупреждение PVS-Studio: 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; // <=
....
}
}
Эта рубрика в наших статьях, кажется, уже стала постоянной. Бессмысленно проверять указатель на nullptr, если память была выделена с помощью оператора new. При невозможности выделить память, оператор new генерирует исключение std::bad_alloc(), а не возвращает nullptr. Чтобы исправить этот код, можно добавить обработчик исключения, либо использовать new (std::nothrow).
Аналогичные предупреждения анализатора:
- 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: 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)))) {
....
}
Такая проверка может быть упрощена:
if (!nodeJuggler ||
(nodeJuggler->isEnded() ||
!nodeJuggler->canMergeAction(actionName))) {
....
}
Аналогичное предупреждение анализатора:
- 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: 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) { // <=
....
}
....
}
Стоит проверить условие цикла for на наличие ошибок. Если ошибок нет, стоит убрать дублирующую проверку.
Аналогичное предупреждение анализатора:
- 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: 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; // <=
....
}
Здесь выделили и освободили память под объект cmd, но так ни разу его и не использовали.
Предупреждение PVS-Studio: 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;
}
В данном примере переменная isRightmost имеет тип bool. Используя унарный минус, переменную неявно преобразовывают к типу int и передают получившееся число в метод adjusted(). Такой код усложняет понимание. Явное лучше, чем неявное, так что, думаю, стоит переписать этот фрагмент так:
QRect KisEqualizerSlider::Private::boundingRect() const
{
QRect bounds = q->rect().adjusted(0, 0, isRightmost ? -1 : 0, -1);
return bounds;
}
Аналогичные предупреждения анализатора:
- 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
Заключение
В заключение мне хочется обратиться к разработчикам Krita и предложить им бесплатно возобновить использование нашего анализатора.
С тех пор, как разработчики последний раз использовали PVS-Studio, у нас появились версии для Linux и MacOS (сам я проверял этот проект в Linux), а также значительно улучшился анализ.
Кроме того, приглашаю всех желающих скачать и попробовать PVS-Studio на коде своего проекта.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Egor Bredikhin. Check of the Krita 4.0 Free Source Code Graphics Editor
Комментарии (20)
32bit_me
23.04.2018 17:35Если присмотреться, то можно заметить, что внутри данного сложного условия находится проверка вида (!a || a):
d->documentLayout->changeTracker()->displayChanges() ||
!d->documentLayout->changeTracker()->displayChanges()
Такое условие является всегда истинным, из-за чего вся эта большая проверка становится бессмысленной, о чем и сообщает анализатор.
Нет же. Здесь происходит вызов displayChanges(), и если резутьтат равен false, то происходит вызов displayChanges() второй раз. Вообще не факт, что он также будет false. Это условие абсолютно не обязано быть истинным.
Для примера, вот код:
int cond() { if(foo() || !foo()) {} }
Вот результат компиляции:
cond(): sub rsp, 8 call foo() test al, al jne .L2 call foo() .L2: add rsp, 8 ret
Ни о каком всегда истинном условии не может быть и речи.EgorBredikhin
23.04.2018 19:35+1Давайте взглянем на код вызываемых методов:
bool KoChangeTracker::displayChanges() const { return d->displayChanges; }
KoChangeTracker *KoTextDocumentLayout::changeTracker() const { return d->changeTracker; }
Как мы видим,changeTracker()
просто возвращает указатель из приватного поляd
, аdisplayChanges()
просто возвращаетbool
из приватного поляd
.
При этом, между вызовами эти значения не изменяются. Анализатор понимает это и выдает предупреждение.
Т.е. в данном случае анализатор производит анализ тел вызываемых методов и собирает информацию о модифицируемых переменных, а вовсе не ограничивается простым паттерн-матчингом видаx || !x
.
Код условия целикомSultansoy
23.04.2018 20:04+1Очень классное поведение для анализатора. Но в статье про исходный код вызвваемых методов ничего сказано не было, поэтому в принципе комментарий выше был правильным. Но поведение анализатор выше всяких похвал.
lany
26.04.2018 03:42Интересно, а если метод виртуальный, вы делаете выводы, что у него нет побочных эффектов на основании реализации? Или смотрите всех доступных наследников? Что если анализируется код библиотеки и предполагается наличие наследников вне зоны видимости анализатора?
dkazakov-dev
23.04.2018 18:17Спасибо, что потестировали и отрепортили на багзиллу! Я посмотрю. При беглом просмотре часть багов явно реальные.
Оправдание 1:
compression.cpp 110 — это не бага, а защитный код, который, возможно, стоит переделать на ассерт. Код PSD сжатия был скопирован из неподдерживаемой более библиотеки, поэтому лишние меры предосторожности не повредят.
False positive 2:
kis_all_filter_test.cpp 154 — это классика жанра, которую уже даже в стандарте С++ обсуждают. Объект cmd реализует RAII для транзакции над полотном изображения, у него очень нетривиальные конструктор с деструктором. Было бы круто поменять варнинг на что-то вроде «зачем выпендриваетесь, создайте уж объект в стеке».
zanac
23.04.2018 22:41kis_all_filter_test.cpp 154 — это классика жанра
Где о ней можно почитать? Как она называется?dkazakov-dev
23.04.2018 22:53Про саму технику вот тут:
en.wikipedia.org/wiki/Resource_acquisition_is_initialization
Обсуждение про стандарт было вот тут (я не помню, чем кончилось в итоге; вроде не сошлись на универсальном заменителе):
groups.google.com/a/isocpp.org/forum/#!topic/std-proposals/a4CRu2KONZ8
PS:
По хорошему, конечно, мы не должны были создавать эту переменную с куче, а просто сделать это в стеке. В этом смысле, хорошо, что анализатор нас предупреждает (такое выделение памяти без QScopedPointer опасно), но просто текст варнинга говорит совсем о другом :)zanac
24.04.2018 06:04Я подумал, что это какая-то модификация RAII…
dkazakov-dev
24.04.2018 12:52Ну в том-то и дело, что при RAII с динамическим выделением он выдает варнинг.
Mingun
23.04.2018 18:32А у PVS-Studio есть автоматические рефакторинги? А то, как у вас в главе Рефакторинг если сделают оптимизацию макроса
SANITY_ZEROS
, как у вас написано, так еще один баг появится (!
лишний справа от!=
)
dkazakov-dev
23.04.2018 18:48Ой, а заменять, как написано в примере, вообще нельзя, ибо переменная m_filterWeights[i].weight[j] — это не bool! Анализатор правильно написал, что можно заменить, обернув все в bool, а автор статьи это не учел. Вообще, достаточно странное предупреждение. Зачем менять условие на менее понятное, в котором проще допустить ошибку?
EgorBredikhin
23.04.2018 20:08+1Да, здесь я сам накосячил, извините :)
Пример неудачный, заменил его другим.dkazakov-dev
26.04.2018 00:51Кстати, заметил, что PVS-Studio совсем не понимает define'ов, которые разворачиваются в какие-либо сложные конструкции со вложением. Там в логе не менее десятка false-positive репортов на конструкцию вроде такой:
LodDataStructImpl *dst = dynamic_cast<LodDataStructImpl*>(_dst); KIS_SAFE_ASSERT_RECOVER_RETURN(dst); KIS_SAFE_ASSERT_RECOVER_RETURN( dst->lodData->levelOfDetail() == defaultBounds->currentLevelOfDetail()); //^ V522 There might be dereferencing of a potential null pointer 'dst'.
Сами ассерты имеют такой вид:
#define KIS_SAFE_ASSERT_RECOVER(cond) if (!(cond) && (kis_safe_assert_recoverable(#cond,__FILE__,__LINE__), true)) #define KIS_SAFE_ASSERT_RECOVER_RETURN(cond) KIS_SAFE_ASSERT_RECOVER(cond) { return; }
jehy
23.04.2018 18:50Спасибо вам огромное, ребят. Каждый пост приносит ощущение, что кто-то заботится о том, чтобы мир стал немного лучше. А обзор криты это вообще чудесно — недавно переехал на неё полностью и счастлив, чудесная штука.
viktprog
24.04.2018 06:00do { state = inflate(&stream, Z_PARTIAL_FLUSH); if(state == Z_STREAM_END) break; if(state != Z_OK) break; } while (stream.avail_out > 0);
Это, очевидно, приводит к аналогичному:
if(state == Z_STREAM_END || state != Z_OK) break;
которое тоже избыточно.
1eqinfinity
24.04.2018 10:42+1Спасибо! Надеюсь, разработчики это прочтут. Каждый раз при обновлении Криты надеюсь, что вот теперь наконец перееду на нее совсем, но нет.
AngReload
Глупый вопрос, конечно: а вы в каком редакторе единорога рисуете? :)
EgorBredikhin
Обычно мы используем GIMP.