?В очередной раз анализатор PVS-Studio оказался внимательнее человека +77



Возьми баг

Изучая предупреждения анализатора PVS-Studio в процессе проверки различных открытых проектов, мы вновь и вновь убеждаемся, сколь полезен может быть этот инструмент. Анализатор кода невероятно внимателен и никогда не устаёт. Он указывает на ошибки, которые ускользают даже при внимательном обзоре кода. Рассмотрим очередной такой случай.

В предыдущий раз я написал аналогичную заметку, изучая исходный код проекта StarEngine: 2D Game Engine. Теперь анализатор показал своё превосходство надо мной в ходе проверки фреймворка Qt.

Последний раз мы проверяли фреймворк Qt в 2014 году. Прошло много времени, проект изменился, а в анализаторе PVS-Studio появилось много новых диагностик. Значит, вполне можно написать очередную статью, чем я и занялся.

Выписывая интересные примеры ошибок, я повстречал вот такой код:

QWindowsCursor::CursorState QWindowsCursor::cursorState()
{
  enum { cursorShowing = 0x1, cursorSuppressed = 0x2 };
  CURSORINFO cursorInfo;
  cursorInfo.cbSize = sizeof(CURSORINFO);
  if (GetCursorInfo(&cursorInfo)) {
    if (cursorInfo.flags & CursorShowing)   // <= V616
  ....
}

Для этого кода PVS-Studio выдал предупреждение:

V616 CWE-480 The 'CursorShowing' named constant with the value of 0 is used in the bitwise operation. qwindowscursor.cpp 669

Для проверки использовалась нестабильная версия PVS-Studio, поэтому моя вера в анализатор дрогнула. «Эх, что-то мы сломали в механизмах обработки неименованных перечислений», — вздохнул я, и выписал этот случай в багтрекер как ошибку, приводящую к ложному срабатыванию.

Я был абсолютно уверен, что ошибается именно анализатор. Ведь всего несколькими строчками выше написано, что константа CursorShowing равна 1.

При этом я старался быть внимательным! Я несколько раз просмотрел код, чтобы убедиться, что анализатор неправ. Я оформил этот фрагмент кода и соответствующее сообщение как ошибку в багтрекере.

Я провёл внимательный обзор этого маленького участка кода и всё равно облажался. Прав именно анализатор, а не человек.

При подробном изучении ситуации выяснилось, что выше объявляется именованная константа cursorShowing, а в условии используется константа CursorShowing. Разница только в первой букве! В одном месте она строчная, а в другом прописная.

Почему код компилируется? Потому, что константа CursorShowing тоже существует. Вот её объявление:

class QWindowsCursor : public QPlatformCursor
{
public:
  enum CursorState {
    CursorShowing,
    CursorHidden,
    CursorSuppressed
  };
  ....
}

Как видите, константа CursorShowing равна 0. Поэтому анализатор PVS-Studio абсолютно прав, сообщая, что условие (cursorInfo.flags & CursorShowing) не имеет смысла. Условие является всегда ложным.

Анализатор нашёл замечательную опечатку. Любите статический анализ кода! :)


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Once again the PVS-Studio analyzer has proved to be more attentive than a person.

Вы можете помочь и перевести немного средств на развитие сайта



Комментарии (55):

  1. Yak52
    /#19130869 / +3

    А была бы CursorShowing в перечислении на втором месте? Тут скорее требуется предупреждение о том, что какие-то наименования идентификаторов отличаются только регистром, точнее «созвучны».

  2. ainar-g
    /#19130941 / +8

    Видимо, программирование на го обучило меня всегда смотреть на первую букву, но я ошибку заметил сразу.

    А вообще да, бывает, что сначала «анализатор не прав», а потом бьёшь себя по лбу. Поэтому предпочитаю, чтобы мои проекты все анализаторы проходили «на ноль».

  3. artemSitnikov
    /#19131051 / +4

    Я так понимаю религия не позволила использовать уже объявленный в <winuser.h>
    #define CURSOR_SHOWING 0x00000001

    Почему такой код вообще прошел ревью.

    • yurisv3
      /#19131327 / +4

      Qt тащемта мультиплатформенный.

      • Jeka178RUS
        /#19131373 / +5

        Фрагмент кода из сугубо виндового файла, так что тут как раз вполне логично использовать системные дефайны, они же используют виндовую структуру CURSORINFO

    • IRainman
      /#19131943 / +1

      Да… и это ещё дополнительные проблемы вносит. Если в API сейчас написано 1, то завтра там может быть 42 :)

      • khim
        /#19133151 / +1

        Нет, не может. Эти константы вкомпилируются в тело программы, а совместимость под Windows — это святое.

        Вот в Linux — да, могло бы быть.

        • amarao
          /#19134805

          А можно про поломку ABI линуксом? Вообще говоря, ядро обязуется поддерживать любой релизный ABI так долго, как его используют (и не ломать).

          • firk
            /#19135211

            ABI и заголовочные файлы это разные вещи
            В новых заголовочных файлах может быть какая-то функция перенаправлена на новую реализацию с новыми константами. То есть старый софт со старым ABI будет вызывать старую функцию со старыми константами, а новоскомпилированный — под тем же названием новую функцию с другими (бывает что с другими форматами структур вообще), которые в новом .h естественно учтены.

            • amarao
              /#19135245

              В ядре такого нет. Новые ioctl/syscall и флаги могут появляться, старые остаются неизменными.

          • khim
            /#19135527

            Тут вечная путаница. Между Linux (ядром) и GNU/Linux (полной операциокой). Ядро/glibc/libstd++ — у них с совместимостью если не прекрасно, то, по крайней мере «очень хорошо». Xы/Wayland — чуть похуже, но в целом терпимо.

            А вот то, что выше (Gtk+ и прочее) — это тихой ужас.

            • amarao
              /#19135617

              Ну так gtk — это всего лишь одна из библиотек. Хочешь — ставишь, хочешь — не ставишь. Она даже не linux-specific, если уж на то пошло.

              • khim
                /#19135753

                Хочешь — ставишь, хочешь — не ставишь.
                Прекрасная идея. И как вы собираетесь писать программу, которая будет адекватно выглядеть у человека с Ubuntu или Fedora? Правильно реагировать на смену темы (светлая/тёмная/etc), на тыкание на иконку в трее (как вы её вообще туда поместите, кстати?) и так далее.

                В том-то и дело, что для человка который пишет программу для себя и не собирается её распространять — в Linux всё нормально.

                Но как только вы попытаетесь сделать хоть что-нибудь для людей — всё, катастрофа, совместимости нет, то что должно занимать часы превращается в многолетнюю борьбу и отбивает всякую охоту что-либо «для этих идиотов» делать…

                • 0xd34df00d
                  /#19136219

                  Qt возьму. Там, кстати, с совместимостью даже слишком хорошо.

                  • khim
                    /#19136407

                    Если вы так в этом уверены, то сделайте хотя бы совсем смешную программу: просто иконка в трее, вы по ней кликаете, открывается менюшка, в которой есть пункт «hello», выбрав который вы откроете окно, где написано… ну пусть тоже «hello». Только с условием, что она будет работать во всех популярных дистрибутивах, которые ныне ещё поддерживаются: от RHEL5 до Ubuntu 18.04/Fedora 28.

                    В Windows такое делается за час и работает (если правильно скомпилировать) и в Windows XP и в Windows 2010. А если взять компилятор постарше — то будет и в Windows 95 работать с теми же исходниками.

                    А куда нас «великолепная совместимость» Qt доведёт?

                    • Antervis
                      /#19137343

                      современный Qt не поддерживает Win XP

                      • khim
                        /#19138501

                        А зачем для поддержки Windows XP нужна Qt? Там можно и на MFC и на чистом Win API всё сделать.

  4. Satim
    /#19131057 / -2

    А вот не было бы регистрозависимости все было бы хорошо)

    • RidgeA
      /#19131145 / +4

      и ТоГдА БЫ каждый пИсАл НАЗВАНИЯ методов КаК еМу ВзДУМАЕтСя

      • Error1024
        /#19131589

        Уж ЛуЧше ТАк, ЧеМ ТрУДНОуловиМый баг. Ах да — есть такая штука, как автоформатирование кода.

      • Golickoff
        /#19134071

        Я работаю с регистронезависимым языком и ни разу не видел ничего кроме CamelStyle. Если есть регламент, его придерживаются все.

    • DistortNeo
      /#19133029 / +1

      Было бы хорошо, если бы ещё enum-ы в C/C++ не замусоривали пространство имён. Ибо должно быть CursorState::Showing и никак иначе. Очень надеюсь, что обычный enum станет deprecated в одной из будущих версий C++.

      • khim
        /#19133155

        Для того, чтобы обычный enum смог стать deprecated нужно, чтобы был действенный механизм переключения. Пока такого нет. auto_ptr смогли выпилить потому только, что он был слишком ущербен и его, по большому счёту, никто и никогда не использовал.

      • Arranticus
        /#19134675

        О, эта ерунда ещё в Delphi мне мешала. Когда на C# переходил, сначала удивился, что тип перечисления обязательно надо указывать, а потом понял, что только так и можно жить.

  5. bugdesigner
    /#19131235 / +1

    Не люблю я "верблюжьи имена". Всегда пишу только в нижнем регистре с разделением подчеркиваниями.

    • Dima_Sharihin
      /#19131521

      Ну, с Qt еще куда ни шло. До очередного мажорного релиза, они, разумеется, вряд ли смогут поменять naming convention (учитывая, что он еще гвоздями к QtQuick прибит).


      А вот кошмар во FreeRTOS — это да, страх и ужас, на который сами разработчики говорят "извините, но мы боимся ломать код всех наших клиентов, потому терпите"

    • dipsy
      /#19131551 / +1

      такоеНаписание яТоже неПонимаю, какМинимум этоНекрасиво.
      Пишу или_так, ИлиТак.
      писать_всё_в одном_стиле конечно_тоже вариант, но_глазу_не за_что_ухватиться при_разборе более-менее длинного_выражения.
      Я, например, комбинирую, названия_классов и ИменаОбъектов.с_именами_функций().

      • Antervis
        /#19132615 / +2

        Я, например

        вот именно, что вы, а не все. А ведь помимо регистровой разницы еще бывают SmurfSmurf SmurfNaming, m_lpcwstrВенгерка, тлкСглсн (CamelCase/snake_case/beercase вариации), различные Способы выденения_ _членов this->класса, СПрефиксы СТипа ССущности (E — Enums, T — Types, etc.). Еще больше усугубляет проблему тот факт, что в стандарте верхний регистр не задействован, а некоторые функции в beercase вместо snake_case. Не хватает единообразия во всем этом зоопарке.

        • popov654
          /#19133309

          На C++ что, всегда так принято именовать константы? Уж лучше бы капсом их именовать — хоть и вырвиглазно, зато на такие грабли никто не наступит.

          Как же просто всё в Java: ClassName, varName, methodName, CONST_NAME.

          Хотя в общем и целом можно константы именовать так же, как и обычные переменные. Тогда не будет видно, что это константа, но зато сразу единый стиль для любой переменной (константной или нет).

          • sand14
            /#19134175

            Как же просто всё в Java: ClassName, varName, methodName, CONST_NAME

            CONST_NAME пришло в Java качестве одного из стилей C.
            На мой взгляд, для Java это смотрится достаточно чужеродно, хотя хорошо уже то, что это стандарт.

            Больше импонирует C#-подход, когда константы выносятся в отдельный статический класс и именуются в PascalCase:
            Path.DirectorySeparatorChar
            Path.AltDirectorySeparatorChar

            Хотя иногда, как в случае того же Path, констатанты могут смешиваться с методами в одном классе.

            Хуже, когда в C# пытаются переносить Java-стандарт объявления констант (встречал в паре проектов): для C# это совсем чужеродно и размывает уже имеющиеся в языке стандарты.

            • Free_ze
              /#19141053

              Как будет выглядеть единственная константа в классе? Всегда нужно мышкой идентификатор ласкать, чтобы отличить константу от свойства? А как быть без полноценной IDE (GitHub, BitBucket и т.п.)?

  6. OYTIS
    /#19131375 / +3

    Я думал там какой-то хитрый UB будет, а там всего лишь опечатка (достаточно заметная, кстати). При чуть более удачном стечении обстоятельств, ее бы заметил и компилятор: совпадение имен с точностью до регистра одного символа — это редкость и плохой стиль (вот тут бы статическому анализатору ругнуться). При чуть менее — ее не заметил бы и PVS — достаточно было бы этой константе оказаться не на нулевой позиции в енуме.

    • datacompboy
      /#19139979 / +1

      Но в этом-то всё и дело! Сколько б ты ни был готов, всё равно самые мелкие мелочи не так уж и заметны — в какой-то момент они лопаются, ты хлопаешь себя по лбу со словами «вот я дурак», а до этого — в упор не видишь.

      • OYTIS
        /#19140379

        Так и PVS не может похвастаться тем, что ловит ошибки такого рода. А на побитовое «или» с нулем или что-то похожее мне и бесплатный clang analyzer ругался.

        • datacompboy
          /#19140521

          Да. Нету в мире абсолюта, кроме водки абсолют.

  7. CyberSoft
    /#19131663 / +1

    Неужели IDE не подсвечивает использования имени? Стало бы сразу видно, что константы не используются

    • SvyatoslavMC
      /#19131789

      Если использовать IDE с плагином PVS-Studio, то подсветит) Правда его не везде ещё «завезли», но мы работаем в этом направлении.

    • 0xd34df00d
      /#19133095

      KDevelop и CLion точно подсвечивают. Ровно по этой причине семантическая подсветка очень полезна.

  8. gasizdat
    /#19132339 / +3

    Я бы заменил в заголовке слово "человека" на слово "Андрея".

    • Andrey2008
      /#19132455 / -1

      Но ведь «Андрей» — ещё тот «Человечище»! По крайней мере в сфере анализа кода. :)

      • Jef239
        /#19132565 / +3

        Ляп-то в PVS-студио исправили? Он собственно в том, что в сообщении нету номера строки и имени файла, где объявлена константа. Отсюда и проблемы у пользователей, включая самих разработчиков.

  9. Antervis
    /#19132509

    Признаться, слабый повод заводить тикет и тем более — писать об этом статью.

    • Andrey2008
      /#19132537 / +3

      Ну как сказать. Я повидал много *****. И раз, я так ошибся, значит в этом что-то есть. :)

  10. Goodkat
    /#19132799

    О, до Qt добрались!
    А с Qt Creator работает?
    Есть несколько десятков живых проектов на Qt под Linux — было бы здорово их проверить.

    • SvyatoslavMC
      /#19132939

      Смотрите на нашем сайте раздел Быстрый старт/Любой проект. Быстрый анализ проекта из консоли без интеграции. В примере команды конвертера указан формат .tasks, который сделан специально для открытия в QtCreator. Предупреждения анализатора просматриваются в Issues Window.

  11. Glays
    /#19133941

    Заметил ошибку с третьего прочтения, даже при условии что обычно работаю с регистронезависимым кодом.
    Должно быть три предупреждения:

    1. Имя не соответствует соглашению о наименовании
    2. Имя имеет регистронезависимое совпадение
    3. Ну и что там сейчас про константу

    • lany
      /#19135721

      Если с такими предупреждениями сунуться в незнакомый проект, который до сих пор форматом имён не заморачивался, то не выплывите.

      • Glays
        /#19136213

        И автоформатер с учётом коллизий имён!
        В любом случае такие вещи должны быть настраиваемыми.

  12. makarov_sa
    /#19134065 / -1

    Подскажите, а зачем в условии вообще часть "& CursorShowing", если CursorShowing это константа и результат всего выражения зависит ТОЛЬКО от cursorInfo.flags?

    • RidgeA
      /#19134091 / +2

      это битовая маска, результат зависит от результата вычисления побитового и

  13. Sazonov
    /#19134839 / +1

    Я добровольно-принудительно задаю правила по всем именованиям в решарпере для всего проекта. Да и вообще считаю хорошей практикой использовать инструменты для проверки codestyle. В моём случае решарпер бы сразу подсветил опасное место.

  14. MShevchenko
    /#19135239

    Используй венгерскую нотацию, Люк!

    • domix32
      /#19135257

      Так это два enum. Нотация не спасет

  15. domix32
    /#19135253

    Вот за что и не люблю enum в C++ — можно указать члена без явного указания его пространства имён.

    • Dima_Sharihin
      /#19136293

      Теперь есть enum class, который напоминает enum из C#