Изучая предупреждения анализатора 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)
ainar-g
19.09.2018 15:20+8Видимо, программирование на го обучило меня всегда смотреть на первую букву, но я ошибку заметил сразу.
А вообще да, бывает, что сначала «анализатор не прав», а потом бьёшь себя по лбу. Поэтому предпочитаю, чтобы мои проекты все анализаторы проходили «на ноль».
artemSitnikov
19.09.2018 15:36+4Я так понимаю религия не позволила использовать уже объявленный в <winuser.h>
#define CURSOR_SHOWING 0x00000001
Почему такой код вообще прошел ревью.yurisv3
19.09.2018 16:28+4Qt тащемта мультиплатформенный.
Jeka178RUS
19.09.2018 16:38+5Фрагмент кода из сугубо виндового файла, так что тут как раз вполне логично использовать системные дефайны, они же используют виндовую структуру CURSORINFO
IRainman
19.09.2018 18:20+1Да… и это ещё дополнительные проблемы вносит. Если в API сейчас написано 1, то завтра там может быть 42 :)
khim
20.09.2018 00:42+1Нет, не может. Эти константы вкомпилируются в тело программы, а совместимость под Windows — это святое.
Вот в Linux — да, могло бы быть.amarao
20.09.2018 11:59А можно про поломку ABI линуксом? Вообще говоря, ядро обязуется поддерживать любой релизный ABI так долго, как его используют (и не ломать).
firk
20.09.2018 13:15ABI и заголовочные файлы это разные вещи
В новых заголовочных файлах может быть какая-то функция перенаправлена на новую реализацию с новыми константами. То есть старый софт со старым ABI будет вызывать старую функцию со старыми константами, а новоскомпилированный — под тем же названием новую функцию с другими (бывает что с другими форматами структур вообще), которые в новом .h естественно учтены.amarao
20.09.2018 13:24В ядре такого нет. Новые ioctl/syscall и флаги могут появляться, старые остаются неизменными.
khim
20.09.2018 14:15Тут вечная путаница. Между Linux (ядром) и GNU/Linux (полной операциокой). Ядро/glibc/libstd++ — у них с совместимостью если не прекрасно, то, по крайней мере «очень хорошо». Xы/Wayland — чуть похуже, но в целом терпимо.
А вот то, что выше (Gtk+ и прочее) — это тихой ужас.amarao
20.09.2018 14:31Ну так gtk — это всего лишь одна из библиотек. Хочешь — ставишь, хочешь — не ставишь. Она даже не linux-specific, если уж на то пошло.
khim
20.09.2018 14:55Хочешь — ставишь, хочешь — не ставишь.
Прекрасная идея. И как вы собираетесь писать программу, которая будет адекватно выглядеть у человека с Ubuntu или Fedora? Правильно реагировать на смену темы (светлая/тёмная/etc), на тыкание на иконку в трее (как вы её вообще туда поместите, кстати?) и так далее.
В том-то и дело, что для человка который пишет программу для себя и не собирается её распространять — в Linux всё нормально.
Но как только вы попытаетесь сделать хоть что-нибудь для людей — всё, катастрофа, совместимости нет, то что должно занимать часы превращается в многолетнюю борьбу и отбивает всякую охоту что-либо «для этих идиотов» делать…0xd34df00d
20.09.2018 16:15Qt возьму. Там, кстати, с совместимостью даже слишком хорошо.
khim
20.09.2018 16:45Если вы так в этом уверены, то сделайте хотя бы совсем смешную программу: просто иконка в трее, вы по ней кликаете, открывается менюшка, в которой есть пункт «hello», выбрав который вы откроете окно, где написано… ну пусть тоже «hello». Только с условием, что она будет работать во всех популярных дистрибутивах, которые ныне ещё поддерживаются: от RHEL5 до Ubuntu 18.04/Fedora 28.
В Windows такое делается за час и работает (если правильно скомпилировать) и в Windows XP и в Windows 2010. А если взять компилятор постарше — то будет и в Windows 95 работать с теми же исходниками.
А куда нас «великолепная совместимость» Qt доведёт?
Satim
19.09.2018 15:37-2А вот не было бы регистрозависимости все было бы хорошо)
RidgeA
19.09.2018 15:52+4и ТоГдА БЫ каждый пИсАл НАЗВАНИЯ методов КаК еМу ВзДУМАЕтСя
Error1024
19.09.2018 17:20Уж ЛуЧше ТАк, ЧеМ ТрУДНОуловиМый баг. Ах да — есть такая штука, как автоформатирование кода.
Golickoff
20.09.2018 10:07Я работаю с регистронезависимым языком и ни разу не видел ничего кроме CamelStyle. Если есть регламент, его придерживаются все.
DistortNeo
19.09.2018 23:58+1Было бы хорошо, если бы ещё enum-ы в C/C++ не замусоривали пространство имён. Ибо должно быть
CursorState::Showingи никак иначе. Очень надеюсь, что обычный enum станет deprecated в одной из будущих версий C++.khim
20.09.2018 00:44Для того, чтобы обычный
enumсмог стать deprecated нужно, чтобы был действенный механизм переключения. Пока такого нет.auto_ptrсмогли выпилить потому только, что он был слишком ущербен и его, по большому счёту, никто и никогда не использовал.
Arranticus
20.09.2018 11:44О, эта ерунда ещё в Delphi мне мешала. Когда на C# переходил, сначала удивился, что тип перечисления обязательно надо указывать, а потом понял, что только так и можно жить.
bugdesigner
19.09.2018 16:10+1Не люблю я "верблюжьи имена". Всегда пишу только в нижнем регистре с разделением подчеркиваниями.
Dima_Sharihin
19.09.2018 17:09Ну, с Qt еще куда ни шло. До очередного мажорного релиза, они, разумеется, вряд ли смогут поменять naming convention (учитывая, что он еще гвоздями к QtQuick прибит).
А вот кошмар во FreeRTOS — это да, страх и ужас, на который сами разработчики говорят "извините, но мы боимся ломать код всех наших клиентов, потому терпите"
dipsy
19.09.2018 17:14+1такоеНаписание яТоже неПонимаю, какМинимум этоНекрасиво.
Пишу или_так, ИлиТак.
писать_всё_в одном_стиле конечно_тоже вариант, но_глазу_не за_что_ухватиться при_разборе более-менее длинного_выражения.
Я, например, комбинирую, названия_классов и ИменаОбъектов.с_именами_функций().Antervis
19.09.2018 22:04+2Я, например
вот именно, что вы, а не все. А ведь помимо регистровой разницы еще бывают SmurfSmurf SmurfNaming, m_lpcwstrВенгерка, тлкСглсн (CamelCase/snake_case/beercase вариации), различные Способы выденения_ _членов this->класса, СПрефиксы СТипа ССущности (E — Enums, T — Types, etc.). Еще больше усугубляет проблему тот факт, что в стандарте верхний регистр не задействован, а некоторые функции в beercase вместо snake_case. Не хватает единообразия во всем этом зоопарке.popov654
20.09.2018 01:54На C++ что, всегда так принято именовать константы? Уж лучше бы капсом их именовать — хоть и вырвиглазно, зато на такие грабли никто не наступит.
Как же просто всё в Java: ClassName, varName, methodName, CONST_NAME.
Хотя в общем и целом можно константы именовать так же, как и обычные переменные. Тогда не будет видно, что это константа, но зато сразу единый стиль для любой переменной (константной или нет).sand14
20.09.2018 10:24Как же просто всё в Java: ClassName, varName, methodName, CONST_NAME
CONST_NAME пришло в Java качестве одного из стилей C.
На мой взгляд, для Java это смотрится достаточно чужеродно, хотя хорошо уже то, что это стандарт.
Больше импонирует C#-подход, когда константы выносятся в отдельный статический класс и именуются в PascalCase:
Path.DirectorySeparatorChar
Path.AltDirectorySeparatorChar
Хотя иногда, как в случае того же Path, констатанты могут смешиваться с методами в одном классе.
Хуже, когда в C# пытаются переносить Java-стандарт объявления констант (встречал в паре проектов): для C# это совсем чужеродно и размывает уже имеющиеся в языке стандарты.Free_ze
21.09.2018 13:50Как будет выглядеть единственная константа в классе? Всегда нужно мышкой идентификатор ласкать, чтобы отличить константу от свойства? А как быть без полноценной IDE (GitHub, BitBucket и т.п.)?
OYTIS
19.09.2018 16:38+3Я думал там какой-то хитрый UB будет, а там всего лишь опечатка (достаточно заметная, кстати). При чуть более удачном стечении обстоятельств, ее бы заметил и компилятор: совпадение имен с точностью до регистра одного символа — это редкость и плохой стиль (вот тут бы статическому анализатору ругнуться). При чуть менее — ее не заметил бы и PVS — достаточно было бы этой константе оказаться не на нулевой позиции в енуме.
datacompboy
21.09.2018 11:30+1Но в этом-то всё и дело! Сколько б ты ни был готов, всё равно самые мелкие мелочи не так уж и заметны — в какой-то момент они лопаются, ты хлопаешь себя по лбу со словами «вот я дурак», а до этого — в упор не видишь.
OYTIS
21.09.2018 12:19Так и PVS не может похвастаться тем, что ловит ошибки такого рода. А на побитовое «или» с нулем или что-то похожее мне и бесплатный clang analyzer ругался.
CyberSoft
19.09.2018 17:31+1Неужели IDE не подсвечивает использования имени? Стало бы сразу видно, что константы не используются
SvyatoslavMC
19.09.2018 17:50Если использовать IDE с плагином PVS-Studio, то подсветит) Правда его не везде ещё «завезли», но мы работаем в этом направлении.
0xd34df00d
20.09.2018 00:24KDevelop и CLion точно подсвечивают. Ровно по этой причине семантическая подсветка очень полезна.
gasizdat
19.09.2018 20:19+3Я бы заменил в заголовке слово "человека" на слово "Андрея".
Andrey2008 Автор
19.09.2018 21:04-1Но ведь «Андрей» — ещё тот «Человечище»! По крайней мере в сфере анализа кода. :)
Jef239
19.09.2018 21:43+3Ляп-то в PVS-студио исправили? Он собственно в том, что в сообщении нету номера строки и имени файла, где объявлена константа. Отсюда и проблемы у пользователей, включая самих разработчиков.
Antervis
19.09.2018 21:25Признаться, слабый повод заводить тикет и тем более — писать об этом статью.
Andrey2008 Автор
19.09.2018 21:36+3Ну как сказать. Я повидал много *****. И раз, я так ошибся, значит в этом что-то есть. :)
Goodkat
19.09.2018 23:00О, до Qt добрались!
А с Qt Creator работает?
Есть несколько десятков живых проектов на Qt под Linux — было бы здорово их проверить.SvyatoslavMC
19.09.2018 23:37Смотрите на нашем сайте раздел Быстрый старт/Любой проект. Быстрый анализ проекта из консоли без интеграции. В примере команды конвертера указан формат .tasks, который сделан специально для открытия в QtCreator. Предупреждения анализатора просматриваются в Issues Window.
Glays
20.09.2018 09:44Заметил ошибку с третьего прочтения, даже при условии что обычно работаю с регистронезависимым кодом.
Должно быть три предупреждения:
- Имя не соответствует соглашению о наименовании
- Имя имеет регистронезависимое совпадение
- Ну и что там сейчас про константу
makarov_sa
20.09.2018 10:06-1Подскажите, а зачем в условии вообще часть "& CursorShowing", если CursorShowing это константа и результат всего выражения зависит ТОЛЬКО от cursorInfo.flags?
Sazonov
20.09.2018 12:04+1Я добровольно-принудительно задаю правила по всем именованиям в решарпере для всего проекта. Да и вообще считаю хорошей практикой использовать инструменты для проверки codestyle. В моём случае решарпер бы сразу подсветил опасное место.
domix32
20.09.2018 13:27Вот за что и не люблю enum в C++ — можно указать члена без явного указания его пространства имён.
Yak52
А была бы CursorShowing в перечислении на втором месте? Тут скорее требуется предупреждение о том, что какие-то наименования идентификаторов отличаются только регистром, точнее «созвучны».