Comments 22
Глупый вопрос, конечно: а вы в каком редакторе единорога рисуете? :)
Если присмотреться, то можно заметить, что внутри данного сложного условия находится проверка вида (!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


Ни о каком всегда истинном условии не может быть и речи.
Давайте взглянем на код вызываемых методов:

bool KoChangeTracker::displayChanges() const
{
    return d->displayChanges;
}


KoChangeTracker *KoTextDocumentLayout::changeTracker() const
{
    return d->changeTracker;
}

Как мы видим, changeTracker() просто возвращает указатель из приватного поля d, а displayChanges() просто возвращает bool из приватного поля d.
При этом, между вызовами эти значения не изменяются. Анализатор понимает это и выдает предупреждение.
Т.е. в данном случае анализатор производит анализ тел вызываемых методов и собирает информацию о модифицируемых переменных, а вовсе не ограничивается простым паттерн-матчингом вида x || !x.

Код условия целиком

Очень классное поведение для анализатора. Но в статье про исходный код вызвваемых методов ничего сказано не было, поэтому в принципе комментарий выше был правильным. Но поведение анализатор выше всяких похвал.

Интересно, а если метод виртуальный, вы делаете выводы, что у него нет побочных эффектов на основании реализации? Или смотрите всех доступных наследников? Что если анализируется код библиотеки и предполагается наличие наследников вне зоны видимости анализатора?

Спасибо, что потестировали и отрепортили на багзиллу! Я посмотрю. При беглом просмотре часть багов явно реальные.

Оправдание 1:
compression.cpp 110 — это не бага, а защитный код, который, возможно, стоит переделать на ассерт. Код PSD сжатия был скопирован из неподдерживаемой более библиотеки, поэтому лишние меры предосторожности не повредят.

False positive 2:
kis_all_filter_test.cpp 154 — это классика жанра, которую уже даже в стандарте С++ обсуждают. Объект cmd реализует RAII для транзакции над полотном изображения, у него очень нетривиальные конструктор с деструктором. Было бы круто поменять варнинг на что-то вроде «зачем выпендриваетесь, создайте уж объект в стеке».

kis_all_filter_test.cpp 154 — это классика жанра

Где о ней можно почитать? Как она называется?
Про саму технику вот тут:
en.wikipedia.org/wiki/Resource_acquisition_is_initialization

Обсуждение про стандарт было вот тут (я не помню, чем кончилось в итоге; вроде не сошлись на универсальном заменителе):
groups.google.com/a/isocpp.org/forum/#!topic/std-proposals/a4CRu2KONZ8

PS:
По хорошему, конечно, мы не должны были создавать эту переменную с куче, а просто сделать это в стеке. В этом смысле, хорошо, что анализатор нас предупреждает (такое выделение памяти без QScopedPointer опасно), но просто текст варнинга говорит совсем о другом :)
Ну в том-то и дело, что при RAII с динамическим выделением он выдает варнинг.

А у PVS-Studio есть автоматические рефакторинги? А то, как у вас в главе Рефакторинг если сделают оптимизацию макроса SANITY_ZEROS, как у вас написано, так еще один баг появится (! лишний справа от !=)

Ой, а заменять, как написано в примере, вообще нельзя, ибо переменная m_filterWeights[i].weight[j] — это не bool! Анализатор правильно написал, что можно заменить, обернув все в bool, а автор статьи это не учел. Вообще, достаточно странное предупреждение. Зачем менять условие на менее понятное, в котором проще допустить ошибку?
Да, здесь я сам накосячил, извините :)
Пример неудачный, заменил его другим.
Кстати, заметил, что 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; }
Насколько я знаю, PVS анализирует препроцессированный код(.i) который содержит у себя уже подставленный код вместо макросов. Это с одной стороны дает возможность отлавливать дополнительные баги в использовании макросов, что не раз приводилось в статьях, с другой стороны много false-positive.
Хм… ну тогда у меня только одно предположение: либо я, либо PVS-Studio не монимает результат выполнения оператора «запятая»:

(kis_safe_assert_recoverable(#cond,__FILE__,__LINE__), true)


По моей логике, выражение под if'ом будет всегда эквивалентно: if (!(cond)), что гарантирует отсутствие разыменования в приведенном примере.
Спасибо вам огромное, ребят. Каждый пост приносит ощущение, что кто-то заботится о том, чтобы мир стал немного лучше. А обзор криты это вообще чудесно — недавно переехал на неё полностью и счастлив, чудесная штука.
    do {
        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;

которое тоже избыточно.

Спасибо! Надеюсь, разработчики это прочтут. Каждый раз при обновлении Криты надеюсь, что вот теперь наконец перееду на нее совсем, но нет.
Only those users with full accounts are able to leave comments. Log in, please.
Information
Founded

1 January 2008

Location

Россия

Employees

31–50 employees

Registered

6 May 2013

Habr blog