Мы используем куки, чтобы пользоваться сайтом
было удобно.
Вы угадали, ответ - "42". Здесь приводится 42 рекомендации по программированию, которые помогут избежать множества ошибок, сэкономить время и нервы. Автором рекомендаций выступает Андрей Карпов - технический директор компании "СиПроВер", разрабатывающей статический анализатор кода PVS-Studio. За свою практику он насмотрелся на огромное количество способов отстрелить себе ногу; ему явно есть, о чем поведать читателю. Каждая рекомендация сопровождается практическим примером, что подтверждает актуальность поднятого вопроса. Советы ориентированы на C/C++ программистов, но часто они универсальны и будут интересны разработчикам, использующим и другие языки.
Здравствуйте. Меня зовут Андрей Карпов. Сфера моих интересов - язык C/C++ и продвижение методологии статического анализа кода. На протяжении пяти лет я являюсь Microsoft MVP в номинации Visual C++. Основная цель моих статей и работы, сделать код программ немножко безопасней и качественней. Буду рад, если этот документ научит вас писать более надежный код и предостережет от некоторых типовых ошибок. Немало полезного здесь можно будет почерпнуть и тем, кто занимается написанием стандартов кодирования для своих компаний.
Немного истории. Не так давно я создал ресурс, на котором делился различными полезными советами по программированию на языке С++. Ресурс не собрал ожидаемое количество подписчиков, поэтому я не вижу смысла приводить здесь на него ссылку. Сайт просуществует какое-то время, после чего уйдет в небытие. А вот советы достойны сохранения. Поэтому я доработал, пополнил эти советы и объединил их в единый текст. Желаю приятного чтения.
Рассмотрим фрагмент кода, позаимствованный из проекта MySQL. Код содержит ошибку, которую анализатор PVS-Studio диагностирует следующим образом: V525 The code containing the collection of similar blocks. Check items '0', '1', '2', '3', '4', '1', '6' in lines 680, 682, 684, 689, 691, 693, 695.
static int rr_cmp(uchar *a,uchar *b)
{
if (a[0] != b[0])
return (int) a[0] - (int) b[0];
if (a[1] != b[1])
return (int) a[1] - (int) b[1];
if (a[2] != b[2])
return (int) a[2] - (int) b[2];
if (a[3] != b[3])
return (int) a[3] - (int) b[3];
if (a[4] != b[4])
return (int) a[4] - (int) b[4];
if (a[5] != b[5])
return (int) a[1] - (int) b[5]; <<<<====
if (a[6] != b[6])
return (int) a[6] - (int) b[6];
return (int) a[7] - (int) b[7];
}Разъяснение
Классическая ошибка, связанная с копированием фрагментов кода (Copy-Paste). По всей видимости, был размножен блок кода "if (a[1] != b[1]) return (int) a[1] - (int) b[1];". Затем начали менять индексы, и в одном месте забыли изменить "1" на "5". В результате функция сравнения будет изредка давать неверный результат, и это будет сложно обнаружить. И её действительно сложно обнаружить, раз она не была выявлена никаким тестами до момента, пока мы не проверили MySQL с помощью PVS-Studio.
Корректный код
if (a[5] != b[5])
return (int) a[5] - (int) b[5];Рекомендация
Хотя код красиво оформлен и легко читается, это не помогло программистам заметить и устранить ошибку. На таком коде человеку сложно сосредоточиться. Он видит однотипные блоки и просматривает их невнимательно.
Эти однотипные блоки скорее всего появились из-за желания максимально оптимизировать код. Программист "развернул цикл" руками. В данном случае, я думаю, это было плохой идеей.
Во-первых, вряд ли программист что-то этим достиг. Современные компиляторы весьма умны и самостоятельно хорошо разворачивают циклы, если это приведёт к ускорению работы программы.
Во-вторых, из-за этой попытки оптимизации в коде появилась ошибка. Если написать простой цикл, то в нём будет гораздо сложнее допустить ошибку.
Я бы порекомендовал переписать эту функцию так:
static int rr_cmp(uchar *a,uchar *b)
{
for (size_t i = 0; i < 7; ++i)
{
if (a[i] != b[i])
return a[i] - b[i];
}
return a[7] - b[7];
}Преимущества:
При этом, я почти уверен, что функция будет работать не медленнее, чем её длинный вариант.
Собственно, рекомендация. Пишите простой и понятный код. Как правило простой код - это правильный код. Не старайтесь взять на себя работу компилятора. Например, не спешите развернуть циклы. Скорее всего, компилятор хорошо справится и без вашей помощи. Заниматься такими мелкими ручными оптимизациями есть смысл только в очень критичных участках кода и только после того, как профилировщик укажет, что данный участок кода является проблемным (медленным).
Следующий фрагмент кода взят из проекта CoreCLR. Код содержит ошибку, которую анализатор PVS-Studio диагностирует следующим образом: V698 Expression 'memcmp(....) == -1' is incorrect. This function can return not only the value '-1', but any negative value. Consider using 'memcmp(....) < 0' instead.
bool operator( )(const GUID& _Key1, const GUID& _Key2) const
{ return memcmp(&_Key1, &_Key2, sizeof(GUID)) == -1; }Разъяснение
Взглянем на описание функции memcmp():
int memcmp ( const void * ptr1, const void * ptr2, size_t num );
Compares the first num bytes of the block of memory pointed by ptr1 to the first num bytes pointed by ptr2, returning zero if they all match or a value different from zero representing which is greater if they do not.
Return value:
Обратите внимание, что если блоки не совпадают, то возвращаются значения больше или меньше нуля. Именно больше или меньше. Это важно! Нельзя сравнивать результат работы таких функций как memcmp(), strcmp(), strncmp() и так далее с константами 1 и -1.
Что интересно, неправильный код, где результат сравнивается с 1/-1 может работать так, как ожидает программист многие годы. Но это везение и не больше того. Поведение функции может самым неожиданным образом поменяться. Например, вы смените компилятор или разработчики новым образом оптимизируют memcmp(), и ваш код перестанет работать.
Корректный код
bool operator( )(const GUID& _Key1, const GUID& _Key2) const
{ return memcmp(&_Key1, &_Key2, sizeof(GUID)) < 0; }Рекомендация
Не полагайтесь на наблюдаемое поведение функций. Если в документации написано, что функция может вернуть значения меньше 0 или больше 0, то так оно и есть. Значит, функция может вернуть -10, 2 или 1024. То, что вы всё время наблюдаете, что функция возвращает -1, 0 или 1, ничего не значит.
Кстати, из того, что функция может вернуть такие числа как, например, 1024 вытекает, что результат работы функции memcmp() нельзя поместить в переменную типа char. Это ещё одна распространённая ошибка, и её последствия могут быть весьма опасны. Одна такая ошибка послужила причиной серьезной уязвимости в MySQL/MariaDB до версий 5.1.61, 5.2.11, 5.3.5, 5.5.22. Суть в том, что при подключении пользователя MySQL /MariaDB вычисляется токен (SHA от пароля и хэша), который сравнивается с ожидаемым значением функцией memcmp(). На некоторых платформах возвращаемое значение может выпадать из диапазона [-128..127]. В итоге, в 1 случае из 256, процедура сравнения хэша с ожидаемым значением всегда возвращает значение true, независимо от хэша. В результате простая команда на bash даёт злоумышленнику рутовый доступ к уязвимому серверу MySQL, даже если он не знает пароль. Причиной этому стал такой код в файле 'sql/password.c':
typedef char my_bool;
...
my_bool check(...) {
return memcmp(...);
}Фрагмент взят из проекта Audacity. Ошибка выявляется PVS-Studio диагностикой: V501 There are identical sub-expressions to the left and to the right of the '-' operator.
sampleCount VoiceKey::OnBackward (....) {
...
int atrend = sgn(buffer[samplesleft - 2]-
buffer[samplesleft - 1]);
int ztrend = sgn(buffer[samplesleft - WindowSizeInt-2]-
buffer[samplesleft - WindowSizeInt-2]);
...
}Разъяснение
Выражение "buffer[samplesleft - WindowSizeInt-2]" вычитается само из себя. Эта ошибка связана с копированием фрагментов кода (Copy-Paste) - строчку скопировали, но забыли исправить в ней константу 2 на 1.
Ошибка банальна до безобразия, но от этого она не перестаёт быть ошибкой. Такие ошибки - суровая реальность программистов, поэтому я не раз буду рассматривать подобные случаи. Я им объявляю войну.
Корректный код
int ztrend = sgn(buffer[samplesleft - WindowSizeInt-2]-
buffer[samplesleft - WindowSizeInt-1]);Рекомендация
Будьте аккуратны и внимательны, дублируя фрагменты кода.
Нет смысла призывать отказаться от копирования участков кода. Это слишком удобно и полезно, чтобы лишать себя такой функциональности в редакторе.
Поэтому просто порекомендую быть аккуратней и не спешить.
Знайте, что копирование кода порождает огромное количество ошибок. Вы только посмотрите, что, например, находится с помощью диагностики V501. Половина этих ошибок - это последствия Copy-Paste.
Если копируешь и правишь код - проверь что получилось! Не ленись!
К проблеме Copy-Paste мы ещё вернемся ниже в этой статье. Я же знаю, что вы всё равно не поняли всю глубину проблемы, но я не дам про неё забыть.
Фрагмент взят из проекта Haiku (преемница операционной системы BeOS). Ошибка выявляется PVS-Studio диагностикой: V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator.
bool IsVisible(bool ancestorsVisible) const
{
int16 showLevel = BView::Private(view).ShowLevel();
return (showLevel - (ancestorsVisible) ? 0 : 1) <= 0;
}Разъяснение
Взглянем на приоритеты операций в языке Си/Си++. У тернарного оператора ?: очень низкий приоритет. Он ниже, чем у операций /, +, < и так далее; ниже он и приоритета оператора минус. В результате программа ведёт себя не так, как хотел программист.
Программист думает, что используется вот такая последовательность операций:
(showLevel - (ancestorsVisible ? 0 : 1) ) <= 0А на самом деле она такая:
((showLevel - ancestorsVisible) ? 0 : 1) <= 0Ошибка допущена в весьма простом коде. Это подчеркивает всю опасность оператора ?:. Используя его, очень легко ошибиться, а использовать тернарный оператор в сложных условиях - это вообще вредительство. Мало того, что легко сделать и не заметить ошибку, так ещё и читать такие выражения бывает очень сложно.
Бойтесь, бойтесь оператора ?:. Я повидал немало ошибок с его участием.
Корректный код
return showLevel - (ancestorsVisible ? 0 : 1) <= 0;Рекомендация
Раньше в статьях я уже обсуждал проблему тернарного оператора, но с тех пор я стал ещё большим параноиком. Как показывает пример приведённый выше, ошибку легко сделать даже в коротком и простом выражении, поэтому я модифицирую свои предыдущие советы.
Совсем отказываться от ?: я не предлагаю. Этот оператор иногда полезен и даже необходим. Тем не менее, прошу им не злоупотреблять, а если вы решили где-то использовать тернарный оператор, то я дам следующую рекомендацию.
ВСЕГДА заключайте тернарный оператор в скобки.
Предположим, у вас есть выражение:
A = B ? 10 : 20;
Теперь записывайте его так:
A = (B ? 10 : 20);
Да, сейчас скобки излишни.
Зато, когда через год вы или ваш коллега захочет добавить к числу 10 или 20 переменную X, ничего не сломается:
A = X + (B ? 10 : 20);
Если бы скобок не было, вы могли забыть про низкий приоритет оператора ?: и испортить программу.
Конечно, можно вписать "X+" внутрь скобок, что приведёт всё к той же ошибке. Но все-таки, это дополнительная защита, и пренебрегать ею не стоит.
Фрагмент взят из проекта LibreOffice. Ошибка выявляется PVS-Studio диагностикой: V718 The 'CreateThread' function should not be called from 'DllMain' function.
BOOL WINAPI DllMain( HINSTANCE hinstDLL,
DWORD fdwReason, LPVOID lpvReserved )
{
....
CreateThread( NULL, 0, ParentMonitorThreadProc,
(LPVOID)dwParentProcessId, 0, &dwThreadId );
....
}Разъяснение
В давние времена я подрабатывал фрилансером. Однажды мне была поставлена задача, с которой я не смог справиться. Задача была некорректная, но тогда я про это не догадывался. При этом, на первый взгляд задание казалось понятным и простым.
При определенном условии в функции DllMain нужно было выполнить ряд действий, используя Windows API функции. Что именно нужно было сделать уже забылось, но что-то простое.
Я потратил уйму времени, но мой код упорно не хотел работать. Причем, если сделать новое обыкновенное приложение, то код работает, а в функции DllMain не работает. Чудеса и загадки. Тогда я так и не разобрался в сути проблемы.
Только сейчас, по прошествии многих лет, разрабатывая анализатор PVS-Studio, я вдруг понял причину той старой неудачи. В функции DllMain можно выполнять только очень ограниченный набор действий. Дело в том, что некоторые DLL могут быть ещё не подгружены и вызывать функции из них нельзя.
Мы сделали диагностику, которая предупреждает программистов, если встречает в функциях DllMain опасные действия. Теперь я понимаю, что у меня была именно такая ситуация.
Подробности
Ситуация с DllMain хорошо описана в статье на сайте MSDN: Dynamic-Link Library Best Practices. Приведу из неё некоторые фрагменты:
При вызове функции DllMain происходит блокировка загрузчика. По этой причине, на функции, которые могут быть вызваны внутри DllMain, накладываются существенные ограничения. Как таковая, функция DllMain предназначена для выполнения задач по минимальной инициализации за счет использования небольшого подмножества Microsoft Windows API. Внутри нее нельзя вызывать функции, которые прямо или косвенно пытаются использовать загрузчик. В противном случае, Вы рискуете создать в программе такие условия, при которых произойдет ее аварийное завершение либо взаимная блокировка потоков. Ошибка в реализации DllMain может подвергнуть опасности весь процесс целиком и все его потоки.
В идеале функция DllMain должна представлять собой всего лишь пустую заглушку. Однако, учитывая сложность многих приложений, данное ограничение было бы слишком строгим, поэтому на практике при работе с этой функцией следует откладывать инициализацию как можно дольше. Отложенная инициализация повышает надежность работы приложения, поскольку она не происходит, пока загрузчик заблокирован. Кроме того, отложенная инициализация позволяет безопасно использовать Windows API в значительно большем объеме.
Некоторые задачи инициализации не могут быть отложены. Например, DLL-библиотека, которая зависит от файла конфигурации, должна прерывать свою загрузку, если файл оказывается неправильно сформированным или содержит мусор. При таком типе инициализации DLL-библиотеки должны предпринимать попытку выполнить запланированное действие и в случае неудачи сразу же завершаться вместо того, чтобы тратить ресурсы, выполняя какую-то другую работу.
В любом случае никогда не выполняйте следующие задачи в пределах функции DllMain:
Корректный код
Приведённый фрагмент кода из проекта LibreOffice может работать, а может и не работать. Всё зависит от везения.
Невозможно легко исправить такую ошибку, требуется рефакторинг кода с целью сделать функцию DllMain максимально простой и короткой.
Рекомендация
Сложно дать рекомендацию. Всё знать невозможно, любой может неожиданно столкнуться с ошибкой такого тайного вида. Формальной рекомендацией должно быть: внимательно читайте всю документацию, относящуюся к тому, с чем работаете, но я думаю вы понимаете, что невозможно заранее предугадывать все такие случаи. Тогда только и будешь, что читать документацию, программировать будет некогда. Даже прочитав N страниц, всё равно не будет уверенности, что ты не прочитал ещё статью X, которая предупредит о беде.
Хочется дать более практичные советы, но, к сожалению, на ум приходит только один: используйте статические анализаторы и иные инструменты анализа кода. Да, это не гарантирует отсутствия ошибок, но от каких-то ошибок всё-таки спасёт. Скажи мне много лет назад анализатор, что в функции DllMain нельзя вызывать функцию Foo, я сэкономил бы массу времени и ещё больше нервных клеток, а ведь тогда я очень расстраивался и злился, так как не мог выполнить поставленную задачу.
Фрагмент взят из проекта IPP Samples. Ошибка выявляется PVS-Studio диагностикой: V205 Explicit conversion of pointer type to 32-bit integer type: (unsigned long)(img)
void write_output_image(...., const Ipp32f *img,
...., const Ipp32s iStep) {
...
img = (Ipp32f*)((unsigned long)(img) + iStep);
...
}Примечание. Когда я ранее в статье приводил этот пример, многие писали в комментариях, что код плох сразу по нескольким причинам. Согласен, но давайте оставим за рамками вопрос, зачем именно так нужно двигаться по буферу данных, и почему код написан так, а не иначе. Сейчас важно, что указатель явно приводится к типу "unsigned long". И только это. Я выбрал этот пример исключительно из-за его краткости.
Разъяснение
Указатель хотят сдвинуть на определённое количество байт. Этот код будет корректно работать в Win32 программе, так как в ней размер указателя совпадает с размером типа long. Однако, если мы скомпилируем 64-битный вариант программы, то указатель станет 64-битным, и при преобразовании его в тип long, будут потеряны значения старших битов.
Примечание. В Linux используется другая модель данных. В 64-битных Linux программах тип 'long' является 64-битным, однако, всё равно плохая идея - использовать 'long' для хранения указателя. Во-первых, такой код нередко попадает в Windows приложения, где является некорректным. Во-вторых, существуют специальные типы, само название которых подразумевает, что в них может храниться указатель. Например, это intptr_t. Использование таких типов облегчает понимание программы при её изучении.
В приведённом примере мы наблюдаем классическую ошибку, которая проявляет себя в 64-битных программах. Сразу подчеркнём, что есть много других ошибок, которые подстерегают программиста на пути создания 64-битного приложения, однако, запись значения указателя в 32-битную целочисленную переменную является самой распространённой и самой коварной ошибкой.
Наглядно этот ошибку можно проиллюстрировать следующим образом:
Рисунок 1. A) 32-битная программа. B) 64-битный указатель ссылается на объект, расположенный в младших адресах. C) 64-битный указатель портится.
Остановлюсь чуть подробнее на коварности. Иногда такие ошибки бывает очень трудно заметить. Программа "почти работает". Ошибки потери старших битов в указателях могут проявляться только через несколько часов активной работы с программой. Сначала память выделяется в младших адресах памяти, поэтому все объекты и массивы лежат в младших 4 гигабайтах памяти. Всё работает хорошо.
В процессе работы приложения память фрагментируется, и даже если программа использует не очень много памяти, новые объекты могут быть созданы за пределами первых 4 гигабайт памяти. Вот здесь и начинаются проблемы; специально повторить возникающие проблемы очень трудно.
Корректный код
Для целочисленного представления указателей можно использовать такие типы как size_t, INT_PTR, DWORD_PTR, intrptr_t, uintptr_t и так далее.
img = (Ipp32f*)((uintptr_t)(img) + iStep);На самом деле, здесь вообще можно было обойтись без явных приведений типов. Нигде не упоминается, что выравнивание отлично от стандартного, т.е. нет никакой магии с использованием __declspec(align( # )) и тому подобного. Значит, указатели сдвигаются на количество байт, кратное размеру Ipp32f; в противном случае, здесь возникнет неопределённое поведение.
Поэтому можно написать так:
img += iStep / sizeof(*img);
Рекомендация
Используйте специальные типы для хранения указателей. Никаких int или long. Наиболее универсальным решением являются следующие типы: intptr_t, uintptr_t. В Visual C++ доступны также следующие типы: INT_PTR, UINT_PTR, LONG_PTR, ULONG_PTR, DWORD_PTR. Само название типа говорит, что в него может быть помещён указатель.
Указатель вполне можно поместить в типы size_t, ptrdiff_t, но рекомендовать это, пожалуй, не стоит. У этих типов другой смысл - они предназначены для хранения размеров и индексов.
В uintptr_t нельзя поместить указатель на функцию-член класса. Функции-члены классов несколько отличаются от стандартных функций. Кроме самого указателя, они хранят скрытое значение this, который указывает на объект класса. Впрочем, это не важно - в 32-битной программе вы не можете положить такой указатель в unsigned int. Такие указатели всегда обрабатываются особым образом, поэтому и проблем с ними в 64-битных программах не возникает. По крайней мере, я таких ошибок не видел.
Если вы собираетесь сделать свою программу 64-битной, в первую очередь следует просмотреть и изменить все фрагменты кода, где указатели преобразовываются в 32-битные целочисленные типы данных. Напомню - в программе будут и другие проблемные места, но начать стоит именно с указателей.
Тем, кто занимается или планирует заниматься созданием 64-битных приложений, дополнительно рекомендую ознакомиться со следующим ресурсом: Разработки 64-битных приложений на языке Си/Си++.
Фрагмент взят из проекта Pixie. Ошибка выявляется PVS-Studio диагностикой: V505 The 'alloca' function is used inside the loop. This can quickly overflow stack.
inline void triangulatePolygon(....) {
...
for (i=1;i<nloops;i++) {
...
do {
...
do {
...
CTriVertex *snVertex =
(CTriVertex *) alloca(2*sizeof(CTriVertex));
...
} while(dVertex != loops[0]);
...
} while(sVertex != loops[i]);
...
}
...
}Разъяснение
Функция alloca(size_t) выделяет память, используя для этого стек. Память, выделенная с помощью alloca(), освобождается при выходе из функции.
Как правило, для программ выделяется не так уж и много стековой памяти. По умолчанию, когда вы создаёте проект в Visual C++, в настройках указано использовать стек размером всего 1 Мегабайт, поэтому функция alloca() очень быстро может исчерпать всю доступную стековую память, если она располагается в теле цикла.
В приведённом выше примере присутствуют сразу 3 вложенных цикла. Таким образом, при триангуляции большого полигона возникнет переполнение стека.
Опасно использовать в циклах и такие макросы, как A2W, так как внутри они так же содержат вызов функции alloca().
Как сказано выше, по умолчанию Windows-программы используют стек размером в 1 Мегабайт. Это значение можно изменить, для этого в настройках проекта найдите и измените параметры Stack Reserve Size и Stack Commit Size. Подробности: "/STACK (Stack Allocations)". Однако, следует понимать, что увеличение размера стека не является решением проблемы - вы только отодвигаете момент, когда стек программы закончится.
Рекомендация
Не вызывайте функцию alloca() внутри циклов. Если у вас есть цикл, и вам нужно выделить временный буфер, то можно предложить 3 варианта:
Фрагмент взят из проекта LibreOffice. Ошибка выявляется PVS-Studio диагностикой: V509 The 'dynamic_cast<T&>' operator should be located inside the try..catch block, as it could potentially generate an exception. Raising exception inside the destructor is illegal.
virtual ~LazyFieldmarkDeleter()
{
dynamic_cast<Fieldmark&>
(*m_pFieldmark.get()).ReleaseDoc(m_pDoc);
}Разъяснение
Если в программе возникает исключение, начинается свертывание стека, в ходе которого объекты разрушаются путем вызова деструкторов. Если деструктор объекта, разрушаемого при свертывании стека, бросает еще одно исключение, и это исключение покидает деструктор, библиотека C++ немедленно аварийно завершает программу, вызывая функцию terminate(). Из этого следует, что деструкторы никогда не должны распространять исключения. Исключение, брошенное внутри деструктора, должно быть обработано внутри того же деструктора.
Приведенный код весьма опасен. Оператор dynamic_cast генерирует исключение std::bad_cast, если не может привести ссылку на объект к нужному типу.
Аналогично опасны любые конструкции, которые могут вызвать исключение. Например, опасно выделять в деструкторе память с помощью оператора new. В случае неудачи, он генерирует исключение std::bad_alloc.
Корректный (безопасный) код
Код можно исправить, применяя оператор dynamic_cast не к ссылке, а к указателю. В этом случае, если преобразовать тип объекта невозможно, он не генерирует исключение, а просто возвращает nullptr.
virtual ~LazyFieldmarkDeleter()
{
auto p = dynamic_cast<Fieldmark*>m_pFieldmark.get();
if (p)
p->ReleaseDoc(m_pDoc);
}Рекомендация
Делайте деструкторы максимально простыми. Деструктор - это не место для выделения памяти или чтения файлов.
Конечно, не всегда можно сделать деструктор простым, но я считаю, что к этому надо стремиться. Более того, сложный деструктор, как правило, говорит о плохом дизайне класса и о непродуманности решений.
Чем больше кода в деструкторе, тем сложнее всё предусмотреть. Становится трудно сказать, какой участок кода может сгенерировать исключение, а какой нет.
Если исключение может возникнуть, то часто хорошим решением является подавить его, используя catch(...):
virtual ~LazyFieldmarkDeleter()
{
try
{
dynamic_cast<Fieldmark&>
(*m_pFieldmark.get()).ReleaseDoc(m_pDoc);
}
catch (...)
{
assert(false);
}
}Да, это может спрятать какую-то ошибку, возникающую в деструкторе, но приложение в целом может работать стабильнее.
Я не буду настаивать, что деструктор ни при каких обстоятельствах не должен генерировать исключение. Всё зависит от ситуации. Иногда сгенерировать исключение в деструкторе полезно. Я встречал такое в узкоспециализированных классах, но это особые случаи. Эти классы по задумке спроектированы так, что при разрушении объекты генерируют исключения, но если это обыкновенный класс, такой как "своя строка", "точка", "кисточка", "треугольник", "документ" и так далее, то такие классы ни в коем случае не должны выбрасывать из деструктора исключение.
Помните, что двойное исключение приводит к останову программы, и примите решение, допустимо это в вашем проекте или нет.
Фрагмент взят из проекта Notepad++. Ошибка выявляется PVS-Studio диагностикой: The error text: V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *headerM != '\0'.
TCHAR headerM[headerSize] = TEXT("");
...
size_t Printer::doPrint(bool justDoIt)
{
...
if (headerM != '\0')
...
}Разъяснение
Благодаря тому, что автор кода использовал для обозначения терминального нуля литерал '\0' можно заметить и исправить ошибку. Автор молодец, по крайне мере частично.
Представим, что было бы если он написал:
if (headerM != 0)Адрес массива проверяется на равенство 0. Сравнение не имеет смысла, так как результат всегда true. Что это? Ошибка или просто лишняя проверка? Ответить сложно, особенно если это чужой код или код, написанный много лет назад.
Однако, мы видим в коде '\0' и начинаем подозревать, что хотели проверить значение одного символа. В добавок, сравнивать указатель headerM с NULL не имеет смысла. Подумав, мы понимаем, что хотели узнать, пустая строка или нет, но ошиблись. Чтобы исправить код, следует добавить разыменование указателя.
Корректный код
TCHAR headerM[headerSize] = TEXT("");
...
size_t Printer::doPrint(bool justDoIt)
{
...
if (*headerM != _T('\0'))
...
}Рекомендация
Число 0 может означать NULL, false, терминальный ноль '\0', просто значение 0, поэтому не ленитесь и не используйте 0 для краткости везде, где только можно. Использование "голого" 0 усложняет чтение кода и мешает находить ошибки.
Используйте:
Это упростит чтение кода и поможет выявить ошибки при обзорах кода.
Фрагмент взят из проекта CoreCLR. Ошибка выявляется PVS-Studio диагностикой: V522 Dereferencing of the null pointer 'hp' might take place.
heap_segment* gc_heap::get_segment_for_loh (size_t size
#ifdef MULTIPLE_HEAPS
, gc_heap* hp
#endif //MULTIPLE_HEAPS
)
{
#ifndef MULTIPLE_HEAPS
gc_heap* hp = 0;
#endif //MULTIPLE_HEAPS
heap_segment* res = hp->get_segment (size, TRUE);
if (res != 0)
{
#ifdef MULTIPLE_HEAPS
heap_segment_heap (res) = hp;
#endif //MULTIPLE_HEAPS
....
}Разъяснение
Я считаю конструкции #ifdef/#endif злом. К сожалению, это неизбежное зло. Они нужны, и все мы их используем, поэтому я не буду призывать вас не использовать #ifdef, это не имеет смысла. Однако, я хочу призвать вас не "частить".
Думаю, многие из читателей сталкивались с кодом, напичканным #ifdef. Особенно тяжкое впечатление производит код, где #ifdef следует через каждые 10 строк кода или ещё чаще. Как правило, это системно-зависимый код, и без #ifdef в нём не обойтись, но от этого не легче.
Обратите внимание, как тяжело читать код, приведённый в примере! А именно чтение кода - это основное занятие программиста. Да, да. Мы намного больше времени читаем код и разбираемся в нем, чем пишем новый текст. Поэтому трудночитаемый текст очень сильно снижает продуктивность работы и увеличивает вероятность появления ошибки.
Вернемся к приведённому выше фрагменту кода. Ошибка в разыменовании нулевого указателя, если не объявлен макрос MULTIPLE_HEAPS. Для простоты я раскрою макросы:
heap_segment* gc_heap::get_segment_for_loh (size_t size)
{
gc_heap* hp = 0;
heap_segment* res = hp->get_segment (size, TRUE);
....Объявили переменную hp, инициализировали её NULL, и тут же разыменовали. Если не объявлен MULTIPLE_HEAPS, то будет беда.
Корректный код
Эта ошибка до сих пор живёт в CoreCLR (12.04.2016) несмотря на то, что мой коллега описал её в статье "25 подозрительных фрагментов кода из CoreCLR". Так что я затрудняюсь точно сказать, как должен выглядеть правильный вариант кода.
На мой взгляд, если (hp == nullptr), то переменную 'res' нужно инициализировать каким-то другим значением. Но я не знаю каким, поэтому на этот раз правильный вариант кода я пропущу.
Рекомендации
Боритесь с мелкими блоками #ifdef/#endif. Они очень затрудняют чтение кода! Код с "лесом" из #ifdef сложно поддерживать и в нём очень легко допустить ошибку.
Советы на все случаи жизни я не дам: решения зависят от ситуации. Главное - помнить, что с #ifdef есть проблема и постоянно стараться оставить код максимально читаемым.
Совет N1. Попробовать отказаться от #ifdef.
Иногда #ifdef можно заменить на константы и обычный оператор if. Сравним 2 участка кода. Вариант с макросами:
#define DO 1
#ifdef DO
static void foo1()
{
zzz();
}
#endif //DO
void F()
{
#ifdef DO
foo1();
#endif // DO
foo2();
}Такой код читать очень тяжело, даже не хочется. Я уверен, вы просто пропустили приведённый фрагмент кода. Сравните теперь вот с таким вариантом:
const bool DO = true;
static void foo1()
{
if (!DO)
return;
zzz();
}
void F()
{
foo1();
foo2();
}Код читается намного проще. Кто-то может возразить, что такой код менее эффективен, так как вызывается функция и осуществляется проверка. Я с ним не согласен. Во-первых, компиляторы сейчас очень умные и в Release версии есть высокая вероятность получить один и тот же код без лишних проверок и вызовов функций. Во-вторых, эти потери слишком малы, чтобы о них думать. Красивый и понятный код важнее.
Совет N2. "Укрупните" блоки #ifdef.
Если бы я писал функцию get_segment_for_loh(), то не стал бы делать в ней несколько #ifdef. Я бы сделал две версии функции. Да, получилось бы чуть больше текста, зато функции будет легко читать и изменять.
Кто-то возразит, что это дублирование кода, мол у меня много больших функций, в которых есть #ifdef. Если делать две функции, то будет дублирование кода. Я буду что-то править в одном варианте функции, и забывать сделать в другом.
Подождите, подождите! А почему у вас большие функции? Вынесите общую логику в отдельные вспомогательные функции, тогда два варианта функции будут короткими и в них хорошо будет видно, чем они отличаются.
Я понимаю, что этот совет не универсален, но подумайте над ним.
Совет N3. Подумайте о шаблонах, возможно, они вам помогут.
Совет N4. Просто подумайте, прежде чем написать #ifdef. Быть может, он не так и нужен? Или можно обойтись меньшим количеством #ifdef, собрав "зло" в одном месте?
Фрагмент взят из проекта Godot Engine. Ошибка выявляется PVS-Studio диагностикой: V567 Undefined behavior. The 't' variable is modified while being used twice between sequence points.
static real_t out(real_t t, real_t b, real_t c, real_t d)
{
return c * ((t = t / d - 1) * t * t + 1) + b;
}Разъяснение
Иногда можно встретить фрагменты кода, где программист попытался поместить как можно больше логики в малый объём кода, создавая при этом сложные конструкции. Навряд ли он этим поможет компилятору, но может легко помешать другому человеку (и даже самому себе) читать и понимать код. К тому же, в таких фрагментах кода легче допустить ошибку.
Достаточно часто ошибки, связанные с неопределённым поведением, как раз и встречаются там, где старались уместить всего побольше в одном месте. Обычно это запись и чтение из одной и той же переменной в рамках одной точки следования. Тут следует несколько более детально остановиться на понятиях "неопределённого поведения" и "точки следования".
Неопределённое поведение - свойство некоторых языков программирования в определённых ситуациях выдавать результат, зависящий от реализации компилятора или ключей оптимизации. Некоторые случаи неопределённого поведения (в частности - этот) тесно связаны с понятием точки следования.
Точка следования - любая точка программы, в которой гарантируется, что все побочные эффекты предыдущих вычислений уже проявились, а побочные эффекты следующих ещё отсутствуют. В языках программирования C\C++ существуют следующие точки следования:
Примечание. В новом стандарте языка С++ разработчики отошли от понятия "точка следования". Однако сейчас, для краткого знакомства воспользуемся именно таким объяснением. Оно проще и достаточно, чтобы пояснить, почему не надо производить много действий в "одной куче".
В данном примере нет ни одной из вышеперечисленных точек следования, а оператор '=', так же как и скобки, точками следования не являются. Таким образом, нельзя сказать, какое значение переменной t будет использовано при вычислении возвращаемого значения.
Ещё раз другими словами - данное выражение представляет собой одну точку следования. Поэтому неизвестно, в каком порядке будут осуществляться доступ к переменной 't'. Например, подвыражение "t * t" может быть вычислено как до записи в переменную "t = t / d - 1", так и после.
Корректный код
static real_t out(real_t t, real_t b, real_t c, real_t d)
{
t = t / d - 1;
return c * (t * t * t + 1) + b;
}Рекомендация
Уместить выражение целиком в одну строчку в приведённом фрагменте кода явно не было лучшей идеей. Мало того, что оно тяжелее читается, так туда ещё закралась ошибка.
Исправив ситуацию и разбив содержимое выражения, мы решили сразу 2 проблемы: улучшили читабельность кода и устранили неопределённое поведение, добавив точку следования.
Конечно, приведённый выше фрагмент кода - не единичный случай:
*(mem+addr++) =
(opcode >= BENCHOPCODES) ? 0x00 : ((addr >> 4)+1) << 4;Как и в прошлый раз, ошибка кроется в усложнении кода на ровном месте. Попытка инкрементировать переменную addr в рамках одного выражения привела к возникновению неопределённого поведения, так как неизвестно, какое значение будет содержать переменная 'addr' в правой части выражения - исходное, или инкрементированное.
Решение проблемы осталось прежним - не заумствовать, а разделить операции на несколько выражений:
*(mem+addr) = (opcode >= BENCHOPCODES) ? 0x00 : ((addr >> 4)+1) << 4;
addr++;Отсюда следует простой, но полезный вывод - не стоит пытаться уместить все действия в минимальное количество выражений. Возможно, стоит разбить код на несколько фрагментов, тем самым как улучшая читабельность кода, так и уменьшая вероятность возникновения ошибки.
В следующий раз, когда будете писать сложные конструкции, подумайте, какова же будет цена их использования, и готовы ли вы её заплатить.
Фрагмент взят из библиотеки Source SDK. Ошибка выявляется PVS-Studio диагностикой: V525 The code containing the collection of similar blocks. Check items 'SetX', 'SetY', 'SetZ', 'SetZ'.
inline void SetX( float val );
inline void SetY( float val );
inline void SetZ( float val );
inline void SetW( float val );
inline void Init( float ix=0, float iy=0,
float iz=0, float iw = 0 )
{
SetX( ix );
SetY( iy );
SetZ( iz );
SetZ( iw );
}Разъяснение
Я уверен, что этот код писался методом Copy-Paste. Копировалась предыдущая строчка, и в ней заменялись отдельные буквы. В самом конце этот метод подвёл - человек утратил внимание и забыл заменить в последней строке букву 'Z' на 'W'.
В рассмотренном примере важно не то, что человек допустил ошибку. Важно, что он допустил её в конце последовательности монотонных действий.
Очень рекомендую ознакомиться с моей статьей "Эффект последней строки". Заметка очень понравилась читателям и позже появился научный вариант этой статьи.
В двух словах: суть в том, что при копировании фрагментов кода, вероятней всего, ошибка будет допущена в самом конце. Это не предположение, а статистические данные.
Корректный код
{
SetX( ix );
SetY( iy );
SetZ( iz );
SetW( iw );
}Рекомендация
Надеюсь вы ознакомились со статьёй, ссылку на которую я привёл выше. Итак, мы имеем следующую ситуацию: при написании однотипного кода, человек копирует фрагменты кода и вносит в них правки. При этом он допускает ошибки, забывая изменить некоторые слова или отдельные символы, наиболее часто он допускает ошибку в конце однотипных действий, так как расслабляется.
Чтобы сократить количество ошибок, могу порекомендовать следующее:
Фрагмент взят из проекта ReactOS (открытая операционная система, совместимая с Windows). Ошибка выявляется PVS-Studio диагностикой: V560 A part of conditional expression is always true: 10035L.
void adns__querysend_tcp(adns_query qu, struct timeval now) {
...
if (!(errno == EAGAIN || EWOULDBLOCK ||
errno == EINTR || errno == ENOSPC ||
errno == ENOBUFS || errno == ENOMEM)) {
...
}Разъяснение
Сейчас приведён маленький фрагмент кода, и найти в нём ошибку несложно, но в реальном коде заметить ошибку весьма проблематично. Взгляд просто пропускает блок однотипных сравнений и идёт дальше.
Причина, почему мы пропускаем такие ошибки в том, что условия плохо отформатированы и не хочется внимательно их читать - это требует усилий. Мы надеемся, что раз проверки однотипные, то всё хорошо, и автор не допустил ошибок в условии.
Один из способов борьбы с опечатками является "табличное" оформление кода.
Для читателей, поленившихся найти ошибку, скажу, что в одном месте пропущено "errno ==". В результате условие всегда истинно, так как константа EWOULDBLOCK не равна нулю.
Корректный код
if (!(errno == EAGAIN || errno == EWOULDBLOCK ||
errno == EINTR || errno == ENOSPC ||
errno == ENOBUFS || errno == ENOMEM)) {Рекомендация
Для начала я приведу код, оформленный самым простым "табличным" способом. Мне он не нравится.
if (!(errno == EAGAIN || EWOULDBLOCK ||
errno == EINTR || errno == ENOSPC ||
errno == ENOBUFS || errno == ENOMEM)) {Стало лучше, но ненамного.
Мне не нравится такой стиль оформления по двум причинам. Во-первых, ошибка по-прежнему не очень заметна. Во-вторых, приходится вставлять большое количество пробелов для выравнивания.
Поэтому надо сделать два усовершенствования в оформлении кода. Первое - не больше одного сравнения на строку, тогда ошибку легко заметить. Смотрите, ошибка стала более заметна:
a == 1 &&
b == 2 &&
c &&
d == 3 &&Второе - рационально писать операторы &&, || и т.д., не справа, а слева.
Смотрите, как много работы для написания пробелов:
x == a &&
y == bbbbb &&
z == cccccccccc &&А вот так работы намного меньше:
x == a
&& y == bbbbb
&& z == ccccccccccВыглядит код немного необычно, но к этому быстро привыкаешь.
Объединим это всё вместе и напишем в новом стиле код, приведённый в самом начале:
if (!( errno == EAGAIN
|| EWOULDBLOCK
|| errno == EINTR
|| errno == ENOSPC
|| errno == ENOBUFS
|| errno == ENOMEM)) {Да, код стал длиннее, зато ошибка стала намного заметнее.
Согласен, смотрится код непривычно. Тем не менее, я рекомендую описанный подход. Я пользуюсь им последний год и весьма доволен, поэтому с уверенностью рекомендую его всем читателям.
То, что код стал длиннее, я вообще не считаю проблемой. Я даже написал бы как-то так:
const bool error = errno == EAGAIN
|| errno == EWOULDBLOCK
|| errno == EINTR
|| errno == ENOSPC
|| errno == ENOBUFS
|| errno == ENOMEM;
if (!error) {Кто-то ворчит что это длинно и загромождает код? Согласен. Так давайте вынесем это в функцию!
static bool IsInterestingError(int errno)
{
return errno == EAGAIN
|| errno == EWOULDBLOCK
|| errno == EINTR
|| errno == ENOSPC
|| errno == ENOBUFS
|| errno == ENOMEM;
}
....
if (!IsInterestingError(errno)) {Может показаться, что я сгущаю краски, и что я слишком перфекционист, однако, ошибки в сложных выражениях очень распространены. Я бы не вспомнил о них, если бы они мне постоянно не попадались: эти ошибки повсюду и плохо заметны.
Вот ещё один пример из проекта WinDjView:
inline bool IsValidChar(int c)
{
return c == 0x9 || 0xA || c == 0xD ||
c >= 0x20 && c <= 0xD7FF ||
c >= 0xE000 && c <= 0xFFFD ||
c >= 0x10000 && c <= 0x10FFFF;
}В функции всего несколько строк, и всё равно в неё закралась ошибка. Функция всегда возвращает true. Вся беда в том, что она плохо оформлена, и многие годы её ленятся читать и заметить там ошибку.
Давайте отрефакторим код в "табличном" стиле, и я бы еще скобочки добавил:
inline bool IsValidChar(int c)
{
return
c == 0x9
|| 0xA
|| c == 0xD
|| (c >= 0x20 && c <= 0xD7FF)
|| (c >= 0xE000 && c <= 0xFFFD)
|| (c >= 0x10000 && c <= 0x10FFFF);
}Не обязательно форматировать код именно так, как я предлагаю. Смысл этой заметки - привлечь ваше внимание к опечаткам в "хаотичном коде". Придавая коду "табличный" вид, можно избежать множество глупых опечаток, и это замечательно. Надеюсь, кому-то эта заметка принесёт пользу.
Ложка дёгтя
Я честный человек, и поэтому должен упомянуть, что иногда форматирование "таблицей" может пойти во вред. Вот один из примеров:
inline
void elxLuminocity(const PixelRGBi& iPixel,
LuminanceCell< PixelRGBi >& oCell)
{
oCell._luminance = 2220*iPixel._red +
7067*iPixel._blue +
0713*iPixel._green;
oCell._pixel = iPixel;
}Это проект eLynx SDK. Программист хотел выровнять код, поэтому перед 713 дописал 0. К сожалению, программист не учёл, что 0 в начале числа означает, что число будет представлено в восьмеричном формате.
Массив строк
В целом концепция форматирования "таблицей" понятна, но давайте закрепим эту тему. С этой целью рассмотрим ещё один случай. Этим я хочу показать, что табличное форматирование стоит применять не только к условиям, а к совершенно разным конструкциям языка.
Фрагмент взят из проекта Asterisk. Ошибка выявляется PVS-Studio диагностикой: V653 A suspicious string consisting of two parts is used for array initialization. It is possible that a comma is missing. Consider inspecting this literal: "KW_INCLUDES" "KW_JUMP".
static char *token_equivs1[] =
{
....
"KW_IF",
"KW_IGNOREPAT",
"KW_INCLUDES"
"KW_JUMP",
"KW_MACRO",
"KW_PATTERN",
....
};Опечатка - забыта одна запятая. В результате две различные по смыслу строки соединяются в одну, т.е. на самом деле здесь написано:
....
"KW_INCLUDESKW_JUMP",
....Ошибку можно было бы избежать, выравнивая код таблицей. Тогда, если запятая будет пропущена, это будет легко заметить.
static char *token_equivs1[] =
{
....
"KW_IF" ,
"KW_IGNOREPAT" ,
"KW_INCLUDES" ,
"KW_JUMP" ,
"KW_MACRO" ,
"KW_PATTERN" ,
....
};Как и в прошлый раз, обращаю внимание, что если мы ставим разделитель справа (в данном случае это запятая), то приходится добавлять массу пробелов, что неудобно. Особенно неудобно, если появляется новая длинная строка/выражение: придётся переформатировать всю таблицу.
Поэтому я вновь рекомендую оформлять эту таблицу так:
static char *token_equivs1[] =
{
....
, "KW_IF"
, "KW_IGNOREPAT"
, "KW_INCLUDES"
, "KW_JUMP"
, "KW_MACRO"
, "KW_PATTERN"
....
};Теперь очень легко заметить недостающую запятую, и нет необходимости писать много пробелов - код красивый и понятный. Быть может, такое оформление непривычно, но к нему быстро привыкаешь, попробуйте.
И под конец короткий лозунг. Как правило, красивый код - это правильный код.
Выше я уже много говорил о хороших стилях кодирования, однако, чтобы сохранять трезвость мышления, рассмотрим антипример. Недостаточно просто писать хороший код: ошибки бывают разнообразнейшие и не всегда от них защищает хороший стиль программирования.
Фрагмент взят из проекта PostgreSQL. Ошибка выявляется PVS-Studio диагностикой: V575 The 'memcmp' function processes '0' elements. Inspect the third argument.
Анализатор Cppcheck тоже умеет выявлять эту ошибку. Он выдаёт сообщение: Invalid memcmp() argument nr 3. A non-boolean value is required.
Datum pg_stat_get_activity(PG_FUNCTION_ARGS)
{
....
if (memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
sizeof(zero_clientaddr) == 0))
....
}Разъяснение
Не там поставлена закрывающаяся скобка, это простая опечатка; к сожалению, она полностью меняет смысл кода.
Выражение sizeof(zero_clientaddr) == 0 всегда равно false, так как размер любого объекта больше 0. Значение false превращается в 0 и в результате функция memcmp() сравнивает 0 байт. Сравнив 0 байт, функция memcmp() считает, что массивы совпадают и возвращает 0. Таким образом, условие, приведенное в примере можно сократить до: if (false).
Корректный код
if (memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
sizeof(zero_clientaddr)) == 0)Рекомендация
Это тот случай, когда очень сложно предложить какую-то практику кодирования, которая защитит от опечаток. Единственное что приходит на ум, это "Yoda conditions", когда константа пишется слева от оператора сравнения:
if (0 == memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
sizeof(zero_clientaddr)))Но я не буду рекомендовать этот стиль - он мне не нравится, и я его не использую. Не нравится он мне по двум причинам:
Во-первых, условия становится труднее читать и понимать. Не знаю, как объяснить, но не зря они названы в честь Йоды :).
Во-вторых, от них нет прока, если говорить о не там поставленных скобах. Есть масса способов допустить ошибку. Вот пример, где используется Yoda conditions, но это не помогло правильно расставить скобки:
if (0 == LoadStringW(hDllInstance, IDS_UNKNOWN_ERROR,
UnknownError,
sizeof(UnknownError) / sizeof(UnknownError[0] -
20)))Эта фрагмент кода из проекта ReactOS. Заметить ошибку сложно, поэтому я выделю её: sizeof(UnknownError[0] - 20).
Итак, Yoda conditions здесь нам не помощник.
Можно придумать какой-то искусственный стиль, где каждая закрывающаяся скобка находится под открывающийся, но это будет длинно, некрасиво и никто так писать не будет.
Таким образом, я не могу порекомендовать какой-то стиль кодирования, чтобы предотвратить не там закрывающиеся скобки.
И здесь, казалось бы, нам мог помочь компилятор. Почему бы ему не предупредить о такой странной конструкции? Но нет - я запускаю Visual Studio 2015, устанавливаю ключ /Wall и не получаю никакого предупреждения. Не будем винить компилятор, у него и без того полно забот.
Главный вывод в этой главе, что не всегда достаточно хорошего стиля кодирования и хорошего компилятора (компилятор в VS2015 я считаю хорошим). Я иногда слышу заявления приблизительно такого рода: "нужно включать предупреждения компилятора на максимум, использовать правильный стиль кодирования, и всё будет хорошо". Нет, это не так. Я не хочу сказать, что кто-то плохо программирует. Но просто все допускают опечатки. Все, без исключения. И многие из этих опечаток прорвутся мимо компилятора и хорошего стиля.
Итак, стиль кодирования + предупреждения компилятора - это важно, но недостаточно. Именно поэтому приходится использовать разнообразные методы поиска ошибок: нет серебряной пули. Качество кода достигается только сочетанием нескольких методологий.
Указанная ошибка может быть найдена с помощью:
Я думаю, читатель понимает, что я всячески продвигаю методологию статического анализа кода. Вот хороший пример, где она крайне полезна и уместна - она способна выявить ошибку на самом раннем этапе - сразу после написания кода.
И действительно, это ошибка легко обнаруживается с помощью таких инструментов как Cppcheck или PVS-Studio.
Вывод. Некоторые не понимают, что недостаточно иметь "прямые руки", чтобы не допускать ошибок. Ошибки допускают все, это неизбежно. Причем, даже супергуру не застрахован от глупых опечаток. Раз это неизбежно, то нет смысла ругаться на программистов, плохой компилятор или плохой стиль - это неконструктивно. Нужно использовать сочетание разных методологий повышения качества программ.
Все имеющиеся у меня примеры, связанные с этой разновидностью ошибок, длинные. Выбрал покороче, но пример всё равно большой, прошу прощения.
Фрагмент взят из библиотеки Source SDK. Ошибка выявляется PVS-Studio диагностикой: V556 The values of different enum types are compared: Reason == PUNTED_BY_CANNON.
enum PhysGunPickup_t
{
PICKED_UP_BY_CANNON,
PUNTED_BY_CANNON,
PICKED_UP_BY_PLAYER,
};
enum PhysGunDrop_t
{
DROPPED_BY_PLAYER,
THROWN_BY_PLAYER,
DROPPED_BY_CANNON,
LAUNCHED_BY_CANNON,
};
void CBreakableProp::OnPhysGunDrop(...., PhysGunDrop_t Reason)
{
....
if( Reason == PUNTED_BY_CANNON )
{
PlayPuntSound();
}
....
}Разъяснение
Переменная 'Reason' является перечислением и имеет тип PhysGunDrop_t. Эту переменную сравнивают с именованной константой PUNTED_BY_CANNON, относящейся совершенно к другому перечислению. Это явная логическая ошибка.
Это весьма распространенная разновидность багов, я встречал их даже в таких проектах, как Clang, TortoiseGit, Linux Kernel.
Причина - в C и старом C++ перечисления не являются типобезопасными. Очень легко запутаться, что с чем сравнивается.
Корректный код
Я не знаю, каким должен быть правильный код. На мой взгляд, PUNTED_BY_CANNON следует заменить на DROPPED_BY_CANNON или LAUNCHED_BY_CANNON. Пусть будет LAUNCHED_BY_CANNON.
if( Reason == LAUNCHED_BY_CANNON )
{
PlayPuntSound();
}Рекомендация
Если вы работаете с С++, то вам повезло - начинайте использовать enum class, и компилятор больше не позволит вам сравнить между собой значения, относящиеся к разным перечислениям. Больше вы случайно не сравните между собой километры и килограммы.
Есть новшества в языке C++, в которых я не уверен. Например, это ключевое слово auto. Мне кажется, его безудержное использование может принести вред, вот мои рассуждения: программист больше времени тратит на чтение, а не на написание кода. Значит, текст программы должен быстро и легко читаться. В языке C переменные объявляются в начале функции, и поэтому, когда вы правите код в середине или конце функции, то сложно понять, что представляет из себя переменная Alice. Вот почему появились различные нотации именования переменных. Например, префиксная где в начале имени сокращенно указан тип переменной, тогда pfAlice может расшифровываться как "указатель на float".
В С++ можно объявлять переменные там, где нужно и это считается хорошим тоном. Префиксы и суффиксы в именах переменных потеряли популярность, и вот появилось ключевое слово auto. Теперь опять в программах мы будем встречать загадочное "auto Alice = Foo();". Alice, who the fuck is Alice?!
Прошу прощения, что отвлекся. Я хотел показать, что некоторые новые возможности могут пойти как на пользу, так и во вред. С enum class это не так, я вижу от этой новой возможности языка только пользу.
Используя enum class, вы должны явно указывать, к какому перечислению относится именованная константа. Это защищает от новых ошибок, т.е. код будет таким:
enum class PhysGunDrop_t
{
DROPPED_BY_PLAYER,
THROWN_BY_PLAYER,
DROPPED_BY_CANNON,
LAUNCHED_BY_CANNON,
};
void CBreakableProp::OnPhysGunDrop(...., PhysGunDrop_t Reason)
{
....
if( Reason == PhysGunDrop_t::LAUNCHED_BY_CANNON )
{
PlayPuntSound();
}
....
}Возможно, поправить старый код будет проблематично, но в новом коде начинайте использовать enum class, не откладывая. Начните прямо с сегодняшнего дня - от этого будет только польза.
Описывать здесь подробно enum class я не вижу смысла, я дам несколько ссылок, которых будет более чем достаточно для знакомства с этой новой замечательной возможностью языка C++11:
Данный раздел пересекается с разделом "не жадничайте на строчках кода", но сейчас я хочу акцентировать внимание немного на другом. Часто создается впечатление, что программисты соревнуются с кем-то невидимым, стремясь написать, как можно более короткий код, что ненужно и вредно.
При этом я не говорю сейчас о каких-то сложных шаблонах. Это вообще отдельная тема, которую я не готов поднимать - слишком сложно провести черту между тем, где шаблоны идут на пользу, а где - во вред. Сейчас я коснусь куда более простой ситуации, она актуальна как для C++, так и C программистов. Все они любят излишне усложнять конструкцию по причине: "я делаю, потому что могу это сделать".
Фрагмент взят из проекта KDE4. Ошибка выявляется PVS-Studio диагностикой: V593 Consider reviewing the expression of the 'A = B == C' kind. The expression is calculated as following: 'A = (B == C)'.
void LDAPProtocol::del( const KUrl &_url, bool )
{
....
if ( (id = mOp.del( usrc.dn() ) == -1) ) {
LDAPErr();
return;
}
ret = mOp.waitForResult( id, -1 );
....
}Разъяснение
У меня возникают вопросы к программистам, которые пишут такой код. Ну и зачем вы это сделали? Вы экономили строчку кода? Вы хотели показать, что умеете совмещать в одном выражении несколько действий?
В результате перед нами - типовой ошибочный паттерн. Он заключается в использовании выражения вида: if (A = Foo() == Error).
Приоритет операции сравнения больше, чем у оператора присваивания, поэтому в начале выполняется сравнение "mOp.del( usrc.dn() ) == -1", а затем переменной id присваивается значение true (1) или false (0).
Если mOp.del() вернёт '-1', то функция завершит свою работу; если не '-1', то работа функции продолжится и значение переменной 'id' будет некорректным. Переменная 'id' всегда будет иметь значение 0.
Корректный код
Хочу подчеркнуть: добавление дополнительных скобок не является решением проблемы. Да, так ошибку можно устранить. Но это неверный путь.
В коде дополнительные скобки были - посмотрите на код внимательно. Сложно сказать, для чего они - возможно, программист боролся с предупреждением компилятора. Возможно, подозревал, что может быть беда с приоритетом операций и хотел скобками исправить ситуацию, но "промазал". В любом случае дополнительная пара скобок ничего не делает.
Поэтому надо смотреть глубже. Давайте не делать код сложнее, если для этого есть возможности. Лучше написать так:
id = mOp.del(usrc.dn());
if ( id == -1 ) {Рекомендация
Не ленитесь написать дополнительную строчку кода, ведь сложные выражения сложно читать. Вначале выполните присваивание, а уже затем проверку - этим облегчите жизнь человеку, который будет сопровождать ваш код, а заодно уменьшите вероятность допустить ошибку.
Попробую подытожить грубо, но кратко: не выпендривайтесь.
Совет в этот раз получился банальным, но надеюсь нелишним. Возможно, он отложится у кого-то в голове, и человек где-то напишет красивый правильный понятный код, а не в стиле "смотрите как я могу!".
Фрагмент взят из проекта Apache HTTP Server. Ошибка выявляется PVS-Studio диагностикой: V597 The compiler could delete the 'memset' function call, which is used to flush 'x' buffer. The RtlSecureZeroMemory() function should be used to erase the private data.
static void MD4Transform(
apr_uint32_t state[4], const unsigned char block[64])
{
apr_uint32_t a = state[0], b = state[1],
c = state[2], d = state[3],
x[APR_MD4_DIGESTSIZE];
....
/* Zeroize sensitive information. */
memset(x, 0, sizeof(x));
}Разъяснение
Для стирания приватных данных используется вызов функции memset(), что неправильно. На самом деле данные стёрты не будут. Вернее сказать, будут они стерты или нет зависит от компилятора, его настроек и фазы луны.
Посмотрите на этот код с точки зрения компилятора. Компилятор старается, чтобы код работал как можно быстрее - для этого он проводит разнообразнейшие оптимизации. Одной из оптимизаций является удаление вызова функций, которые с точки зрения языка C/C++ не влияют на поведение программы. В приведённом коде функция memset() с точки зрения языка C/C++ лишняя. Функция меняет буфер 'x', но этот буфер более нигде не используется, и значит, можно и нужно удалить вызов функции memset().
Важно! Я описываю не теоретическое, а практическое поведения компилятора. В подобных случаях компилятор действительно удаляет вызов функции memset(), вы можете сами провести эксперименты и убедиться в этом. Подробнее эта тема и соответствующие примеры приведены с следующих статьях:
Особенно коварна ошибка с исчезновением memset() тем, что её сложно заметить. Если вы запустите отладчик, то скорее всего будете работать с неоптимизированным кодом, и вызов функции будет на своём месте. Заметить ошибку можно только изучая ассемблерный листинг, полученный при сборке оптимизированной версии приложения.
Некоторые программисты по непонятным мне причинам заявляют, что это баг компилятора, и что компилятор не имеет права выбрасывать вызов такой важной функции как memset(). Это не так. Эта функция ничем не лучше и не хуже, чем другие, и компилятор вправе оптимизировать код, где есть её вызов, в конце концов, этот вызов может быть действительно лишним.
Корректный код
memset_s(x, sizeof(x), 0, sizeof(x));Или
RtlSecureZeroMemory(x, sizeof(x));Рекомендация
Следует использовать специализированные функции очистки памяти, которые не могут быть удалены компилятором в процессе оптимизации кода.
В Visual Studio, например, можно использовать RtlSecureZeroMemory. Начиная с C11, существует функция memset_s. В случае необходимости вы можете создать свою собственную безопасную функцию, в интернете достаточно много примеров, как её сделать. Вот некоторые из вариантов:
Вариант N1.
errno_t memset_s(void *v, rsize_t smax, int c, rsize_t n) {
if (v == NULL) return EINVAL;
if (smax > RSIZE_MAX) return EINVAL;
if (n > smax) return EINVAL;
volatile unsigned char *p = v;
while (smax-- && n--) {
*p++ = c;
}
return 0;
}Вариант N2.
void secure_zero(void *s, size_t n)
{
volatile char *p = s;
while (n--) *p++ = 0;
}Некоторые идут дальше и делают функции, которые заполняют массив псевдослучайными значениями и при этом работают различное время, чтобы затруднить атаки, связанные с замером времени. Их реализацию также можно найти в интернете.
Фрагмент взят из проекта Putty. Неэффективный код выявляется PVS-Studio диагностикой: V814 Decreased performance. Calls to the 'strlen' function have being made multiple times when a condition for the loop's continuation was calculated.
static void tell_str(FILE * stream, char *str)
{
unsigned int i;
for (i = 0; i < strlen(str); ++i)
tell_char(stream, str[i]);
}Разъяснение
Здесь нет ошибки, но подобный код может быть крайне неэффективным, если обрабатываются длинные строки. Функция strlen() вызывается на каждой итерации цикла.
Как правило, такой код пишут люди, работавшие ранее с языком Pascal (средой Delphi). В Pascal граница окончания цикла вычисляется один раз, поэтому, там подобный код уместен и распространён.
Для прояснения ситуации рассмотрим пример кода на языке Pascal. Слово called будет распечатано только один раз, так как функция pstrlen() только один раз и вызывается.
program test;
var
i : integer;
str : string;
function pstrlen(str : string): integer;
begin
writeln('called');
pstrlen := Length(str);
end;
begin
str := 'a pascal string';
for i:= 1 to pstrlen(str) do
writeln(str[i]);
end.Эффективный код
static void tell_str(FILE * stream, char *str)
{
size_t i;
const size_t len = strlen(str);
for (i = 0; i < len; ++i)
tell_char(stream, str[i]);
}Рекомендация
Не забывайте, что в С/С++ условие остановки цикла вычисляется каждый раз заново. Поэтому плохая идея вызывать в условии медленные функции, если это можно сделать один раз заранее. Не жадничайте на промежуточных переменных.
В некоторых случаях, компилятор, возможно, сможет оптимизировать код с strlen(). Например, если указатель всегда ссылается на один и тот же строковый литерал; однако, полагаться на это ни в коем случае нельзя.
Фрагмент взят из проекта LibreOffice. Ошибка выявляется PVS-Studio диагностикой: V603 The object was created but it is not being used. If you wish to call constructor, 'this->Guess::Guess(....)' should be used.
Guess::Guess()
{
language_str = DEFAULT_LANGUAGE;
country_str = DEFAULT_COUNTRY;
encoding_str = DEFAULT_ENCODING;
}
Guess::Guess(const char * guess_str)
{
Guess();
....
}Разъяснение
Хорошие программисты не любят писать дублирующийся код - это хорошо. При этом, когда дело доходит до конструкторов, многие в попытке сделать код коротким и красивым, отстреливают себе ногу.
Дело в том, что конструктор нельзя просто взять и вызвать, как обыкновенную функцию. Если мы напишем "A::A(int x) { A(); }", то вместо вызова конструктора без параметров, будет создан временный неименованный объект типа A.
Именно это и происходит в приведённом выше примере. Создаётся временный объект Guess(), который не имеет имени, после чего он тут же разрушается. Члены класса (а именно, language_str, country_str, encoding_str) остаются неинициализированными.
Корректный код
Раньше существовало 3 способа, как не дублировать код в конструкторах. Рассмотрим их.
Первый вариант: сделать отдельную функцию инициализации и вызывать её из обоих конструкторов. Писать код не буду, так как думаю этот способ очевиден.
Это хороший, надёжный, понятный, безопасный способ, но некоторые нехорошие программисты хотят ещё большей краткости кода, поэтому я вынужден упомянуть ещё два метода.
Два следующих способа весьма опасны и требуется очень хорошо понимать, как они работают и какие могут быть последствия.
Второй вариант:
Guess::Guess(const char * guess_str)
{
new (this) Guess();
....
}Третий вариант:
Guess::Guess(const char * guess_str)
{
this->Guess();
....
}Второй и третий вариант опасны тем, что базовые классы инициализируются дважды. Такой код может спровоцировать трудноуловимые ошибки и принести больше вреда, чем пользы. Рассмотрим пример, где такой вызов конструктора допустим, а где - нет.
Вот случай, где всё хорошо:
class SomeClass
{
int x, y;
public:
SomeClass() { new (this) SomeClass(0,0); }
SomeClass(int xx, int yy) : x(xx), y(yy) {}
};Код безопасен и работает, так как класс содержит только простые типы данных и не наследуется от других классов. Двойной вызов конструктора ничем не грозит.
Рассмотрим другой код, где явный вызов конструктора приводит к ошибке:
class Base
{
public:
char *ptr;
std::vector vect;
Base() { ptr = new char[1000]; }
~Base() { delete [] ptr; }
};
class Derived : Base
{
Derived(Foo foo) { }
Derived(Bar bar) {
new (this) Derived(bar.foo);
}
Derived(Bar bar, int) {
this->Derived(bar.foo);
}
}Итак, мы явно вызываем конструктор с помощью конструкции "new (this) Derived(bar.foo);" или "this->Derived(bar.foo)".
В этот момент подкласс Base уже создан, и его поля инициализированы. Повторный вызов конструктора приведет к двойной инициализации. В ptr будет записан указатель на вновь выделенный участок памяти. В результате получаем утечку памяти. К чему приведет двойная инициализация объекта типа std::vector - вообще предсказать сложно. Ясно одно: такой код недопустим.
И нужна вам такая головная боль? Если вам не доступны возможности C++11, то используйте способ N1 (создайте функцию инициализации). Явный вызов конструктора требуется только в крайне редких случаях.
Рекомендация
К счастью для C++ программистов, настали светлые времена, и жить нам стало проще!
Новый стандарт C++11 позволяет вызывать одни конструкторы класса из других (так называемая делегация). Это позволяет писать конструкторы, использующие поведение других конструкторов, без внесения дублирующего кода.
Пример корректного кода:
Guess::Guess(const char * guess_str) : Guess()
{
....
}Подробнее с делегирующий конструкторами можно познакомиться, изучив следующие ссылки:
Фрагмент взят из проекта SETI@home. Ошибка выявляется PVS-Studio диагностикой: V663 Infinite loop is possible. The 'cin.eof()' condition is insufficient to break from the loop. Consider adding the 'cin.fail()' function call to the conditional expression.
template <typename T>
std::istream &operator >>(std::istream &i, sqlblob<T> &b)
{
....
while (!i.eof())
{
i >> tmp;
buf+=(tmp+' ');
}
....
}Разъяснение
Операция считывания данных из потокового объекта не так тривиальна, как кажется на первый взгляд. Как правило, при считывании данных из потока, программисты проверяют, достигнут ли конец потока, путём вызова метода eof(). Однако, эта проверка неполноценна, так как не является достаточной и не сообщает об ошибках считывания, или же о нарушении целостности потока, что влечёт за собой возникновение потенциальных проблем.
Примечание. Сведения, предоставленные в данной заметке, актуальны как для потоков ввода, так и вывода. Поэтому, чтобы избежать дублирования, здесь приводится информация только по одному типу потока.
Эту ошибку и допустил программист в вышеприведённом коде. В случае, если возникла какая-то ошибка считывания данных, возможно возникновение бесконечного цикла, так как метод eof() будет всегда возвращать значение false. В добавок, в цикле начнут обрабатываться некорректные данные, так как неизвестно какие значения будут попадать в переменную tmp.
Во избежание подобной ситуации необходимо использовать дополнительные методы для проверки состояния потока: bad(), fail().
Корректный код
Воспользуемся тем, что поток может неявно приводиться к типу bool. Значение true говорит, что значение успешно считано. Подробнее работа этого кода рассмотрена в обсуждении на Stack Overfow.
template <typename T>
std::istream &operator >>(std::istream &i, sqlblob<T> &b)
{
....
while (i >> tmp)
{
buf+=(tmp+' ');
}
....
}Рекомендация
При считывании данных из потока, не ограничивайтесь вызовом метода eof(). Дополнительно проверяйте наличие сбоя.
Вы можете использовать методы bad() и fail() для проверки состояния потока. Первый метод указывает на порчу целостности потока, второй сообщает об ошибке чтения данных.
Однако, на практике обычно удобнее использовать оператор bool(), как это показано в примере корректного кода.
Продолжим тему работы с файлами. И вновь обратим внимание на EOF. Но теперь нас ожидает паттерн ошибки совсем иного рода. Проявляет он себя как правило в локализованных версиях программ.
Фрагмент взят из проекта Computational Network Toolkit. Ошибка выявляется PVS-Studio диагностикой: V739 EOF should not be compared with a value of the 'char' type. The 'c' should be of the 'int' type.
string fgetstring(FILE* f)
{
string res;
for (;;)
{
char c = (char) fgetc(f);
if (c == EOF)
RuntimeError("error reading .... 0: %s", strerror(errno));
if (c == 0)
break;
res.push_back(c);
}
return res;
}Разъяснение
Рассмотрим, как объявлен EOF:
#define EOF (-1)Как видите, EOF есть ничто иное как '-1' типа int. Функция fgetc() возвращает значения типа int. А именно - она может вернуть число от 0 до 255 или -1 (EOF). Прочитанные значение помещаются в переменную типа char, из-за этого символ со значением 0xFF (255) превращается в -1 и интерпретируется точно также как конец файла (EOF).
Пользователи, использующие Extended ASCII Codes, иногда сталкиваются с ошибкой, когда один из символов их алфавита некорректно обрабатывается программами.
Например, последняя буква русского алфавита в кодировке Windows-1251 как раз имеет код 0xFF и воспринимается некоторыми программами как конец файла.
Корректный код
for (;;)
{
int c = fgetc(f);
if (c == EOF)
RuntimeError("error reading .... 0: %s", strerror(errno));
if (c == 0)
break;
res.push_back(static_cast<char>(c));
}Рекомендация
Какой-то особенной рекомендации я здесь не дам, но раз речь зашла о EOF, я решил показать интересный вариант ошибки, о существовании которой многие даже не подозревают.
Просто запомните, что если функции возвращают значения типа int, то не спешите превратить его в char. Остановитесь и проверьте, что всё хорошо. Кстати, с подобной ситуацией мы уже сталкивались, когда обсуждали функцию memcmp() в главе N2 "Больше 0, это не 1" (см. фрагмент про уязвимость MySQL).
Фрагмент взят из проекта TortoiseGIT. Ошибка выявляется PVS-Studio диагностикой: V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead.
#pragma warning(disable:4996)
LONG result = regKey.QueryValue(buf, _T(""), &buf_size);
#pragma warning(default:4996)Разъяснение
Программисты часто считают, что после директивы "pragma warning(default : X)" опять начнут действовать предупреждения, отключенные ранее помощью "pragma warning(disable: X)". Это не так. Директива 'pragma warning(default : X)' устанавливает предупреждение с номером 'X' в состояние, которое действует ПО УМОЛЧАНИЮ, а это далеко не одно и то же.
Предположим, что файл компилируется с ключом /Wall. В этом случае, должно выдаваться предупреждение C4061. Если написать "#pragma warning(default : 4061)", то это предупреждение перестанет выдаваться, так как по умолчанию оно является отключенным.
Корректный код
#pragma warning(push)
#pragma warning(disable:4996)
LONG result = regKey.QueryValue(buf, _T(""), &buf_size);
#pragma warning(pop)Рекомендация
Правильным способом возвращения предыдущего состояние предупреждения является совместное использование "#pragma warning(push[ ,n ])" и "#pragma warning(pop)". С описанием этих директив можно познакомиться в документации к Visual C++: Pragma Directives. Warnings.
Особенное внимание рассматриваемой рекомендации должны уделять разработчики библиотек. Неаккуратная работа с настройками предупреждений может вызвать массу неприятных моментов у пользователей такой библиотеки.
Для закрепления данной темы рекомендую дополнительно познакомиться с хорошей статьёй: "Итак, вы хотите заглушить это предупреждение в Visual C++".
Фрагмент взят из библиотеки OpenSSL. Ошибка выявляется PVS-Studio диагностикой: V666 Consider inspecting third argument of the function 'strncmp'. It is possible that the value does not correspond with the length of a string which was passed with the second argument.
if (!strncmp(vstart, "ASCII", 5))
arg->format = ASN1_GEN_FORMAT_ASCII;
else if (!strncmp(vstart, "UTF8", 4))
arg->format = ASN1_GEN_FORMAT_UTF8;
else if (!strncmp(vstart, "HEX", 3))
arg->format = ASN1_GEN_FORMAT_HEX;
else if (!strncmp(vstart, "BITLIST", 3))
arg->format = ASN1_GEN_FORMAT_BITLIST;
else
....Разъяснение
Очень тяжело заставить себя не использовать магические числа. Я даже не буду призывать полностью отказаться от них; вдобавок, избавляться от таких констант как 0, 1, -1, 10 будет непрактично. Придумать имена для таких констант крайне сложно, и часто эти имена будут только усложнять чтение кода.
Однако, стремиться сократить количество магических чисел - дело полезное. Например, стоит избавиться от магических чисел, задающих длину строковых литералов.
Посмотрим на код, показанный выше. Скорее всего писался с помощью методики Copy-Paste. Была скопирована строка
else if (!strncmp(vstart, "HEX", 3))После чего в ней "HEX" заменили на "BITLIST", но забыли заменить 3 на 7. В результате строка сравнивается не с "BITLIST", а только с "BIT". Пожалуй, в данном случае эту ошибку нельзя назвать серьезной, однако все равно это ошибка.
Плохо, что код писался с помощью Copy-Paste, но ещё хуже, что длина строки задавалась магической константой. Мы время от времени встречаем ошибки, когда из-за опечатки или невнимательности длина строки не совпадает с указанным количеством символов. Так что это типовая ошибка и с ней надо что-то делать, поэтому порассуждаем, как можно защититься от таких ошибок.
Корректный код
В первый момент может показаться, что достаточно заменить вызов strncmp() на strcmp(). Тогда магическая константа исчезнет сама собой.
else if (!strcmp(vstart, "HEX"))Жаль, но это неправильно - мы изменили логику работы кода. Функции strncmp() проверяет, что строка начинается с "HEX", а функция strcmp() проверяет, что строки равны. Это разные проверки.
Самый простой способ исправить код - это изменить константу:
else if (!strncmp(vstart, "BITLIST", 7))
arg->format = ASN1_GEN_FORMAT_BITLIST;Это правильный код, но он очень плох, так как в нем по-прежнему присутствует магическая 7. Поэтому я буду рекомендовать исправлять код совсем иным способом.
Рекомендация
Ошибки можно было бы избежать, если явно вычислять длину строки в коде. Самый простой вариант - использовать функцию strlen():
else if (!strncmp(vstart, "BITLIST", strlen("BITLIST")))В этом случае будет проще заметить несоответствие, если забыть поправить одну из строчек:
else if (!strncmp(vstart, "BITLIST", strlen("HEX")))У предложенного варианта есть два недостатка:
С первым недостатком можно бороться, используя специальные конструкции для вычисления длины литерала на этапе компиляции. Например, можно использовать макрос вида:
#define StrLiteralLen(arg) ((sizeof(arg) / sizeof(arg[0])) - 1)
....
else if (!strncmp(vstart, "BITLIST", StrLiteralLen("BITLIST")))Это конструкция опасна. Случайно в процессе рефакторинга может получится код следующего вида:
const char *StringA = "BITLIST";
if (!strncmp(vstart, StringA, StrLiteralLen(StringA)))В этом случае макрос StrLiteralLen вернёт белиберду - мы получим значение 3 или 7 в зависимости от размера указателя (4 или 8 байт). От этой неприятной неожиданности в языке С++ можно защититься, используя более сложную конструкцию:
template <typename T, size_t N>
char (&ArraySizeHelper(T (&array)[N]))[N];
#define StrLiteralLen(str) (sizeof(ArraySizeHelper(str)) - 1)Теперь, если аргументом макроса StrLiteralLen будет простой указатель, код не удастся скомпилировать.
Рассмотрим второй недостаток (дублирование строкового литерала). Я не знаю, что посоветовать программистам на языке С. Можно придумать специальный макрос, но мне не нравится этот вариант, не люблю макросы. Потому не знаю, что советовать.
В случае С++ всё прекрасно. Более того, мы сразу изящно решаем и первую проблему. Нам поможет шаблонная функция сравнения строк; написать её можно по-разному, но в целом она будет выглядеть так:
template<typename T, size_t N>
int mystrncmp(const T *a, const T (&b)[N])
{
return _tcsnccmp(a, b, N - 1);
}Теперь строковый литерал указывается только один раз. Длина строкового литерала вычисляется на этапе компиляции, в функцию нельзя случайно передать простой указатель и неправильно вычислить длину строки. Красота!
Подытожу. Старайтесь не допускать магических чисел при работе со строками. Используйте макросы или шаблонные функции, чтобы по возможности их избегать, тогда код станет не только надёжней, но чаще ещё короче и красивее.
В качестве примера можно взглянуть на объявление функции strcpy_s():
errno_t strcpy_s(
char *strDestination,
size_t numberOfElements,
const char *strSource
);
template <size_t size>
errno_t strcpy_s(
char (&strDestination)[size],
const char *strSource
); // C++ onlyПервый вариант предназначен для языка C или для случая, когда длина буфера заранее неизвестна. Если же мы работаем с буфером, созданным на стеке, то в С++ коде мы можем использовать второй вариант:
char str[BUF_SIZE];
strcpy_s(str, "foo");Нет никаких магических чисел, здесь вообще нет вычисления длины буфера. Это коротко и красиво.
Фрагмент взят из библиотеки MFC. Ошибка выявляется PVS-Studio диагностикой: V301 Unexpected function overloading behavior. See first argument of function 'WinHelpW' in derived class 'CFrameWndEx' and base class 'CWnd'.
class CWnd : public CCmdTarget {
....
virtual void WinHelp(DWORD_PTR dwData,
UINT nCmd = HELP_CONTEXT);
....
};
class CFrameWnd : public CWnd {
....
};
class CFrameWndEx : public CFrameWnd {
....
virtual void WinHelp(DWORD dwData,
UINT nCmd = HELP_CONTEXT);
....
};Разъяснение
При переопределении виртуальной функции достаточно легко сделать ошибку в сигнатуре и определить новую функцию, которая никак не будет связана с функцией в базовом классе. Варианты такой ошибки могут быть самые разные:
Ошибка может возникнуть при изменении типов или количества параметров в существующем коде, когда программист поменял сигнатуру виртуальной функции почти во всей иерархии, но забыл это сделать в каком-то наследнике.
Часто эта ошибка проявляет себя при портировании программы на 64-битную платформу при замене типа DWORD на DWORD_PTR, LONG на LONG_PTR и т.д. Подробности. Мы как раз и имеем дело с таким случаем.
В случае рассмотренной ошибки, 32-битная версия программы будет работать корректно. В ней и DWORD, и DWORD_PTR являются синонимами unsigned long. В 64-битной версии программы случится беда: в ней тип DWORD_PTR уже представляет собой unsigned __int64.
Корректный код
class CFrameWndEx : public CFrameWnd {
....
virtual void WinHelp(DWORD_PTR dwData,
UINT nCmd = HELP_CONTEXT) override;
....
};Рекомендация
Теперь есть способ защититься от описанной ошибки. В C++11 были добавлены два новых спецификатора:
Для нас сейчас интересен спецификатор override. Он является указанием компилятору проверить, что виртуальная функция действительно переопределяет функцию базового класса и выдать ошибку, если это не так.
Если бы override использовали при определении функции WinHelp в классе CFrameWndEx, то произошла бы ошибка компиляции 64-битной версии приложения. Тем самым ошибка была бы предотвращена на самом раннем этапе.
Всегда используйте спецификатор override (или final) при переопределении виртуальной функции. Подробнее про override и final можно прочитать здесь:
Фрагмент взят из проекта CoreCLR. Опасный код выявляется PVS-Studio диагностикой: V704 'this == nullptr' expression should be avoided - this expression is always false on newer compilers, because 'this' pointer can never be NULL.
bool FieldSeqNode::IsFirstElemFieldSeq()
{
if (this == nullptr)
return false;
return m_fieldHnd == FieldSeqStore::FirstElemPseudoField;
}Разъяснение
Раньше часто сравнивали this с 0 / NULL / nullptr. Во времена первого C++ это вообще было стандартным случаем. Мы обнаружили такие места, проводя археологические исследования. Предлагаю почитать о них в заметке о проверке Cfront. Более того, в те времена значение указателя this можно было даже изменять, но это было так давно, что уже все забыли.
Вернемся к сравнению this с nullptr.
Я пришел сообщить вам: теперь такой код вне закона. В современных стандартах языка C++ считается, что this НИКОГДА не может быть равен nullptr.
Формально вызов метода IsFirstElemFieldSeq() для нулевого указателя this по стандарту С++ ведёт к неопределённому поведению.
Кажется, что если this==0, то во время работы метода не производится доступа к полям этого класса, и всё хорошо. На самом деле, существует два возможных неблагоприятных сценария выполнения данного кода. Согласно стандарту C++, указатель this никогда не может быть нулевым. Следовательно, компилятор имеет полное право оптимизировать метод, упростив его до:
bool FieldSeqNode::IsFirstElemFieldSeq()
{
return m_fieldHnd == FieldSeqStore::FirstElemPseudoField;
}Есть и другой подводный камень. Пусть существует следующая иерархия наследования:
class X: public Y, public FieldSeqNode { .... };
....
X * nullX = NULL;
X->IsFirstElemFieldSeq();Предположим, что размер класса Y составляет 8 байт. Тогда исходный указатель NULL (0x00000000) будет скорректирован таким образом, чтобы указывать на начало подобъекта класса FieldSeqNode. Для этого его надо сместить на sizeof(Y) байт. Таким образом, this внутри функции IsFirstElemFieldSeq() будет равен 0x00000008. Проверка "this == 0" полностью потеряла смысл.
Корректный код
Привести корректный код я затрудняюсь - убрать проверку из функции недостаточно. Дополнительно нужно провести рефакторинг кода так, чтобы никогда не вызывать функции, используя нулевой указатель.
Рекомендация
Итак, конструкция вида if (this == nullptr) теперь вне закона. Однако, такой код можно часто встретить во многих приложениях и библиотеках. Например, в библиотеке MFC. Поэтому компилятор Visual C++ по-прежнему, честно сравнивает this с 0. Разработчики компиляторов не настолько сумасшедшие, чтобы сразу сделать так, чтобы старый код, служивший верой и правдой десяток лет, вдруг перестал работать.
Однако, как говорится, закон принят, и поэтому давайте для начала перестанем сравнивать this с нулём. А когда есть свободное время, будет полезно поискать все незаконные сравнения и переписать код.
Скорее всего, будет так: вначале компиляторы начнут выдавать предупреждения на сравнения, возможно, уже выдают, я не изучал этот вопрос. Потом в какой-то момент полноценно поддержат стандарт, и ваш код перестанет работать, поэтому призываю начать исполнять закон, со временем это пригодится.
P.S. При рефакторинге кода вам может пригодиться паттерн Null Object.
Дополнительные ссылки по теме:
Фрагмент взят из проекта MAME. Код содержит ошибку, которую анализатор PVS-Studio диагностирует следующим образом: V721 The VARIANT_BOOL type is utilized incorrectly. The true value (VARIANT_TRUE) is defined as -1. Inspect the first argument.
virtual HRESULT __stdcall
put_HandleKeyboard (VARIANT_BOOL pVal) = 0;
....
pController->put_HandleKeyboard(true);Разъяснение
Есть вот такое забавное высказывание:
Все мы расплачиваемся за первородный грех - изучение Basic в особо впечатлительном возрасте. (c) Ф. Дж. Плоджер.
Данная глава как раз на тему древнего зла. Тип VARIANT_BOOL пришел к нам из Visual Basic. С этим типом связаны наши программистские неприятности. Дело в том, что "истина" в нём кодируется как -1.
Посмотрим на объявление типа и констант, означающих истина/ложь:
typedef short VARIANT_BOOL;
#define VARIANT_TRUE ((VARIANT_BOOL)-1)
#define VARIANT_FALSE ((VARIANT_BOOL)0)
Вроде как ничего страшного в этом нет. Ложь это 0. А истина, это не 0. Так что -1 вполне подходящая константа. Однако слишком легко ошибиться и вместо VARIANT_TRUE использовать true или TRUE.
Корректный код
pController->put_HandleKeyboard(VARIANT_TRUE);Рекомендация
Если вы видите незнакомый тип, лучше не спешить и свериться с документацией. Даже если в своем названии тип содержит слово BOOL, это не значит, что в переменную этого типа можно смело помещать 1.
Аналогично, программисты ошибаются, используя тип HRESULT, норовя сравнить его с FALSE или с TRUE, забывая, что:
#define S_OK ((HRESULT)0L)
#define S_FALSE ((HRESULT)1L)Так что прошу быть аккуратными с непривычными типами и не спешить при программировании.
Поговорим ещё об одном гадком типе данных - BSTR (Basic string or binary string).
Фрагмент взят из проекта VirtualBox. Код содержит ошибку, которую анализатор PVS-Studio диагностирует следующим образом: V745 A 'wchar_t *' type string is incorrectly converted to 'BSTR' type string. Consider using 'SysAllocString' function.
....
HRESULT EventClassID(BSTR bstrEventClassID);
....
hr = pIEventSubscription->put_EventClassID(
L"{d5978630-5b9f-11d1-8dd2-00aa004abd5e}");Разъяснение
Вот как объявлен тип BSTR:
typedef wchar_t OLECHAR;
typedef OLECHAR * BSTR;На первый взгляд кажется, что "wchar_t *" и BSTR это одно и то же. Но это не так, и с этим связано много путаницы и ошибок.
Чтобы лучше понять, в чем собственно дело, поговорим о типе BSTR.
На самом деле, я процитирую статью с сайта MSDN. Читать MSDN документацию не любят, но это нужно сделать.
BSTR (basic string или binary string) - это строковый тип данных, который используется в COM, Automation и Interop функциях. Тип BSTR следует использовать во всех интерфейсах. Представление BSTR:
Тип BSTR является указателем, который указывает на первый символ строки, а не на префикс длины.
Память для BSTR-строк выделяется с помощью функций выделения памяти COM, поэтому они могут возвращаться методами без необходимости контроля над выделением памяти.
Представленный ниже код является неправильным:
BSTR MyBstr = L"I am a happy BSTR";Данный пример собирается (компилируется и линкуется) успешно, но не будет работать должным образом, поскольку у строки отсутствует префикс длины. Если проверить расположение в памяти данной переменной с помощью отладчика, он покажет отсутствие префикса длины размером 4 байта перед началом строки данных.
Правильный вариант кода должен выглядеть так:
BSTR MyBstr = SysAllocString(L"I am a happy BSTR");Теперь отладчик покажет наличие префикса длины, который равен значению 34. Оно соответствует 17 символам, которые приводится к wide-character строке с помощью строкового модификатора "L". Отладчик также покажет двухбайтовый символ-ограничитель (0x0000) в конце строки.
Если передать простую Unicode-строку в качестве аргумента функции COM, которая ожидает BSTR-строку, произойдет сбой в работе этой функции.
Надеюсь, процитированного фрагмента MSDN достаточно, чтобы понять, почему следует разделять BSTR и простые строки типа "wchar_t *".
Дополнительные ссылки:
Корректный код
hr = pIEventSubscription->put_EventClassID(
SysAllocString(L"{d5978630-5b9f-11d1-8dd2-00aa004abd5e}"));Рекомендация
Совет идентичен предыдущему. Если вы видите незнакомый тип, лучше не спешить и свериться с документацией. Совет важный, так что думаю ничего страшного, что я повторил его.
Фрагмент взят из проекта ReactOS. Код содержит ошибку, которую анализатор PVS-Studio диагностирует следующим образом: V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing.
#define stat64_to_stat(buf64, buf) \
buf->st_dev = (buf64)->st_dev; \
buf->st_ino = (buf64)->st_ino; \
buf->st_mode = (buf64)->st_mode; \
buf->st_nlink = (buf64)->st_nlink; \
buf->st_uid = (buf64)->st_uid; \
buf->st_gid = (buf64)->st_gid; \
buf->st_rdev = (buf64)->st_rdev; \
buf->st_size = (_off_t)(buf64)->st_size; \
buf->st_atime = (time_t)(buf64)->st_atime; \
buf->st_mtime = (time_t)(buf64)->st_mtime; \
buf->st_ctime = (time_t)(buf64)->st_ctime; \
int CDECL _tstat(const _TCHAR* path, struct _stat * buf)
{
int ret;
struct __stat64 buf64;
ret = _tstat64(path, &buf64);
if (!ret)
stat64_to_stat(&buf64, buf);
return ret;
}Разъяснение
В этот раз пример кода получился длинноватым. К счастью, он простой, так что думаю не составит проблем его понять.
Идея состояла в следующем: если удалось получить информацию о файле с помощью функции _tstat64(), то помещаем эту информацию в структуру типа _stat. Для пересылки данных предназначен макрос stat64_to_stat.
Реализован макрос плохо. Операции, которые он выполняет, не объединены в блок с помощью фигурных скобок { }. В результате телом условного оператора является только первая строка макроса. Если раскрыть макрос, вот что мы получим:
if (!ret)
buf->st_dev = (&buf64)->st_dev;
buf->st_ino = (&buf64)->st_ino;
buf->st_mode = (&buf64)->st_mode;В результате большинство членов структуры копируются независимо от того, успешно получена информация о файле или нет.
Это ошибка, но с практической точки зрения она не фатальна, просто зря копируются неинициализированные ячейки памяти. Так сказать, "повезло". Я встречал и более серьезные ошибки, связанные с подобными неудачными макросами.
Корректный код
Самый просто вариант - это добавить фигурные скобки в макрос. Вариант чуть получше - добавить do { .... } while (0). Тогда после макроса, как и после функции можно написать точку с запятой ';'.
#define stat64_to_stat(buf64, buf) \
do { \
buf->st_dev = (buf64)->st_dev; \
buf->st_ino = (buf64)->st_ino; \
buf->st_mode = (buf64)->st_mode; \
buf->st_nlink = (buf64)->st_nlink; \
buf->st_uid = (buf64)->st_uid; \
buf->st_gid = (buf64)->st_gid; \
buf->st_rdev = (buf64)->st_rdev; \
buf->st_size = (_off_t)(buf64)->st_size; \
buf->st_atime = (time_t)(buf64)->st_atime; \
buf->st_mtime = (time_t)(buf64)->st_mtime; \
buf->st_ctime = (time_t)(buf64)->st_ctime; \
} while (0)Рекомендация
Я очень не люблю макросы. Я знаю, что без них нельзя обойтись, особенно в языке C, тем не менее, я всячески стараюсь избегать макросы в своём коде и призываю других ими не злоупотреблять. Моя нелюбовь проистекает из 3 причин:
С макросами связаны различные ошибки. Та которую я описал, хорошо показывает, что иногда макросы вообще не нужны. Я не понимаю, что помешало авторам написать обыкновенную функцию. Преимущество функции перед макросом:
Ища недостатки от использования функции, можно разве что завести разговор об оптимизации. Происходят затраты на вызов функции, но это несерьезно.
Однако, предположим, что это крайне важно и пофилософствуем об оптимизации. Во-первых, существует ключевое слово inline и им можно воспользоваться. Во-вторых, уместно объявить функцию как static. Думаю, этого будет достаточно, чтобы компилятор встроил функцию и не делал для неё отдельное тело.
На самом деле, вообще не надо об этом задумываться - компиляторы сейчас стали умными. Даже если вы сделаете просто функцию без всяких inline/static, компилятор может встроить её, если почитает это полезным. Не заморачивайтесь с такими мелочами, лучше пишите простой и понятный код, пользы будет больше.
На мой взгляд код следовало написать так:
static void stat64_to_stat(const struct __stat64 *buf64,
struct _stat *buf)
{
buf->st_dev = buf64->st_dev;
buf->st_ino = buf64->st_ino;
buf->st_mode = buf64->st_mode;
buf->st_nlink = buf64->st_nlink;
buf->st_uid = buf64->st_uid;
buf->st_gid = buf64->st_gid;
buf->st_rdev = buf64->st_rdev;
buf->st_size = (_off_t)buf64->st_size;
buf->st_atime = (time_t)buf64->st_atime;
buf->st_mtime = (time_t)buf64->st_mtime;
buf->st_ctime = (time_t)buf64->st_ctime;
}На самом деле можно сделать ещё несколько улучшений. Например, в С++ лучше передавать не указатель, а ссылку, некрасиво смотрится использование указателей без их предварительной проверки, но это уже выходит за рамки обсуждения макросов.
Фрагмент взят из проекта Unreal Engine 4. Неэффективный код выявляется PVS-Studio диагностикой: V803 Decreased performance. In case 'itr' is iterator it's more effective to use prefix form of increment. Replace iterator++ with ++iterator.
void FSlateNotificationManager::GetWindows(....) const
{
for( auto Iter(NotificationLists.CreateConstIterator());
Iter; Iter++ )
{
TSharedPtr<SNotificationList> NotificationList = *Iter;
....
}
}Разъяснение
Думаю, если не читать названия статьи, недочёт было бы найти весьма тяжело. В данном случае код корректен, но не оптимален. Да, речь о постфиксном инкременте итератора - 'Iter++'. Вместо постфиксной формы инкремента итератора следует использовать префиксный аналог, то есть следует заменить 'Iter++' на '++Iter'. Почему, и есть ли в этом практическая польза? Об этом ниже.
Эффективный код
for( auto Iter(NotificationLists.CreateConstIterator());
Iter; ++Iter)Рекомендация
Чем отличаются префиксные и постфиксные формы записи знают все. Какие отличия между их внутренним строением (откуда вытекает принцип работы, как следствие), тоже, надеюсь, все знают. Если вы занимались перегрузкой операторов, то знать обязаны. Если же нет - поясню (остальные могут перейти к абзацу, следующему за примерами кода с перегрузкой операторов).
Префиксный оператор сначала изменяет состояние объекта, после возвращая себя в изменённом виде. Понятно, что никаких временных объектов не требуется. Тогда оператор префиксного инкремента может выглядеть так:
MyOwnClass& operator++()
{
++meOwnField;
return (*this);
}Постфиксный же оператор также изменяет состояние объекта, но при этом возвращает предыдущее состояние объекта. Каким образом это достигается? Путём создания временного объекта. Тогда код перегрузки оператора постфиксного инкремента может выглядеть так:
MyOwnClass operator++(int)
{
MyOWnCLass tmp = *this;
++(*this);
return tmp;
}Из этих фрагментов кода видно, что в случае постфиксного инкремента используется дополнительная операция создания временного объекта. Насколько это критично на практике?
Компиляторы сейчас достаточно интеллектуальны для того, чтобы самостоятельно производить оптимизацию и не создавать временные объекты, если они не нужны. Поэтому на практике в Release-версии сложно заметить разницу между 'it++' и '++it'
Но совсем иначе дело обстоит при отладке программ в Debug-режиме. В таком случае разница в производительности может оказаться очень и очень значительной.
Например, в этой статье приводятся замеры времени выполнения кода с использованием префиксной и постфиксной форм операторов инкремента в Debug-версии, и она отличается в 4 раза, что весьма немало.
Те, кто скажут: "Ну и что? В Release всё равно всё будет работать одинаково" - будут одновременно правы и неправы. Как правило, большую часть времени приходится иметь дело с Debug-версией. Обычно при разработке запускается Debug версия, отладка производится в Debug, Unit-тесты - обычно тоже исполняются в Debug-версии. Следовательно, приличная часть времени расходуется на работу с Debug-версией ПО, а значит, лишние временные затраты ни к чему.
Думаю, нам удалось ответить на вопрос "Стоит ли использовать префиксный оператор инкремента для итератора вместо постфиксного?". Да, стоит. Лишних сил программист на это не потратит, но получит небольшое ускорение в отладочной версии, а если итераторы "тяжелые", то выигрыш может быть весьма заметен и полезен.
Дополнительные ссылки по теме:
Фрагмент взят из проекта Energy Checker SDK. Код содержит ошибку, которую анализатор PVS-Studio диагностирует следующим образом: V576 Incorrect format. Consider checking the second actual argument of the 'wprintf' function. The pointer to string of wchar_t type symbols is expected.
int main(void) {
...
char *p = NULL;
...
wprintf(
_T("Using power link directory: %s\n"),
p
);
...
}Разъяснение
Примечание. Первая ошибка в том, что используется _T, чтобы задать строку в широком формате. Правильно здесь будет использовать префикс L. Однако, эта ошибка не существенна и для нас сейчас не интересна - код просто не скомпилируется, если мы не захотим использовать широкий формат строк и _T раскроется в пустоту.
Чтобы функция wprintf() распечатала строку типа char *, нужно указать в строке форматирования "%S".
Многие Linux программисты не понимают в чем подвох. Всё дело в том, что Microsoft нестандартно реализовал поведение таких функций как wsprintf(). Если мы работаем в Visual C++ с функцией wsprintf(), то для печати широких строк мы должны использовать "%s", а для печати char* строк, нужен "%S". В общем, шиворот навыворот. Те, кто пишут кроссплатформенные приложения часто попадают в эту ловушку.
Корректный код
Предложенное ниже исправление никак нельзя назвать рекомендуемым или красивым. Привожу его только для того, чтобы пояснить суть необходимых исправлений.
char *p = NULL;
...
#ifdef defined(_WIN32)
wprintf(L"Using power link directory: %S\n"), p);
#else
wprintf(L"Using power link directory: %s\n"), p);
#endifРекомендация
Какого-то особенного совета у меня нет, просто хотел предупредить о неожиданностях, с которыми вы можете столкнуться, используя такие функции как wprintf().
Начиная с Visual Studio 2015 предлагается решение этой проблемы, чтобы писать переносимый код. Для совместимости с ISO C (C99) следует указать препроцессору макрос _CRT_STDIO_ISO_WIDE_SPECIFIERS.
В этом случае, код:
const wchar_t *p = L"abcdef";
const char *x = "xyz";
wprintf(L"%S %s", p, x);является правильным.
Анализатор знает про _CRT_STDIO_ISO_WIDE_SPECIFIERS и учитывает его при анализе.
Кстати, если вы включили режим совместимости с ISO C (объявлен макрос _CRT_STDIO_ISO_WIDE_SPECIFIERS), вы можете в отдельных местах вернуть старое приведение, используя спецификатор формата "%Ts".
Вся эта история с широкими символами достаточно запутанная, и мне не хочется глубоко в неё погружаться. Тем, кто пожелает глубже разобраться в этой теме, предлагаю ознакомиться со следующими ссылками:
Фрагмент взят из игры Wolf. Код содержит ошибку, которую анализатор PVS-Studio диагностирует следующим образом: V511 The sizeof() operator returns size of the pointer, and not of the array, in 'sizeof (src)' expression.
ID_INLINE mat3_t::mat3_t( float src[ 3 ][ 3 ] ) {
memcpy( mat, src, sizeof( src ) );
}Разъяснение
Программисты иногда забывают, что в C/C++ нельзя передать в функцию массив по значению. На самом деле, в качестве аргумента передается указатель на массив. Числа в квадратных скобках ничего не значат, они всего лишь служат, своего рода подсказкой программисту, массив какого размера предполагается передать. На самом деле, передать туда можно массив совсем другого размера. Например, следующий код будет успешно скомпилирован:
void F(int p[10]) { }
void G()
{
int p[3];
F(p);
}Соответственно, оператор sizeof(src) вычисляет не размер массива, а размер указателя. В результате функция memcpy() скопирует только часть массива; а именно 4 или 8 байт, в зависимости от размера указателя (экзотические архитектуры не в счёт).
Корректный код
Самый простой вариант правильного кода может быть таким:
ID_INLINE mat3_t::mat3_t( float src[ 3 ][ 3 ] ) {
memcpy(mat, src, sizeof(float) * 3 * 3);
}Рекомендация
Есть несколько направлений, в которых можно двигаться, чтобы сделать код более безопасным.
Размер массива заранее известен. Вы можете сделать, чтобы функция принимала ссылку на массив. Не все знают, что так можно делать и тем более как это написать. Поэтому, надеюсь, для многих этот пример будет интересен и полезен:
ID_INLINE mat3_t::mat3_t( float (&src)[3][3] )
{
memcpy( mat, src, sizeof( src ) );
}Теперь передать в функцию можно будет только массив правильной размерности. И главное, оператор sizeof() будет вычислять размер массива, а не указателя.
Ещё один вариант решение проблемы - это начать использовать класс std::array.
Размер массива заранее не известен. В умных книгах советуют использовать класс std::vector и подобные ему, однако, на практике это не всегда удобно.
Иногда хочется работать с простым указателем; тогда в функцию необходимо передавать два аргумента: указатель и количество элементов. Это плохо, так как подобный стиль провоцирует массу ошибок.
Для таких случаев могут быть полезны размышления, приводимые в "C++ Core Guidelines". Предлагаю познакомиться с "Do not pass an array as a single pointer". Да и вообще, рекомендую почитать в свободное время "C++ Core Guidelines" - там много интересного и полезного.
Фрагмент взят из проекта TortoiseSVN. Код содержит ошибку, которую анализатор PVS-Studio диагностирует следующим образом: V618 It's dangerous to call the 'printf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str);
BOOL CPOFile::ParseFile(....)
{
....
printf(File.getloc().name().c_str());
....
}Разъяснение
Когда нужно распечатать или, например, записать строку в файл, многие программисты, не задумываясь, пишут код следующего вида:
printf(str);
fprintf(file, str);Это ужасно опасные конструкции. Дело в том, что если внутрь строки каким-то образом попадет спецификатор форматирования, то это приведёт к непредсказуемым последствиям.
Вернемся к изначальному примеру. Если у файла будет имя "file%s%i%s.txt", то программа может упасть или распечатать белиберду.
Но это только пол беды. Фактически такой вызов функции - это самая настоящая уязвимость - с её помощью можно пытаться атаковать программы. Подготовив особенным образом строки, можно распечатать приватные данные, хранимые в памяти.
Подробнее про подобные уязвимости можно прочитать в этой статье. Найдите время с ней познакомиться, думаю, будет интересно. Там не только рассматривается теория, но и приводятся практические примеры.
Корректный код
printf("%s", File.getloc().name().c_str());Рекомендация
Такие функции, как printf() крайне опасны. Лучше вообще их не использовать, а воспользоваться чем-то более современным. Например, Вам могут пригодиться boost::format или std::stringstream.
Помните, неаккуратное использование функций printf(), sprintf(), fprintf() и так далее, может привести к неправильной работе программы. Что ещё более важно, неверное использование этих функций может сделать программу уязвимой к некоторым видам атак.
Фрагмент взят из проекта GIT. Код содержит ошибку, которую анализатор PVS-Studio диагностирует следующим образом: V595 The 'tree' pointer was utilized before it was verified against nullptr. Check lines: 134, 136.
void mark_tree_uninteresting(struct tree *tree)
{
struct object *obj = &tree->object;
if (!tree)
return;
....
}Разъяснение
То, что разыменовывать нулевой указатель - плохо, понятно всем. Результат такого разыменования - неопределенное поведение программы. С теорией все согласны.
Но когда дело доходит до практики, начинаются странные и непонятные дискуссии. Появляются люди, которые начинают утверждать, что вот конкретно такой код будет работать правильно. Они зуб дают за это - у них всегда это работало. А я всегда борюсь с этими людьми, и сейчас я сделаю очередную попытку переубедить их.
Я специально выбрал такой тип примера, который порождает дискуссию. После разыменования указателя 'tree', член класса не используется, а вычисляется адрес этого члена. Затем, если (tree==nullptr), то происходит выход из функции и адрес члена класса никак не используется. Многие считают такой код корректным.
Это не так! Так писать нельзя. Неопределенное поведение - это не обязательно падение программы при записи значения по нулевому адресу и тому подобное. Неопределённое поведение - это всё что угодно. Как только вы разыменовали указатель равный нулю, у вас неопределенное поведение. Рассуждать дальше, как будет работать программа, не имеет смысла - дальше она может работать, как угодно.
Одним из проявлений неопределённого поведения, например, может быть то, что компилятор в процессе оптимизации удалит проверку "if (!tree) return;". Ведь выше указатель разыменовывается, значит указатель не равен нулю. Вывод - проверку можно удалить; и это только один из множества сценариев, почему программа может сломаться.
Рекомендую познакомиться со статьей, где всё это разобрано более подробно: http://www.viva64.com/ru/b/0306/
Корректный код
void mark_tree_uninteresting(struct tree *tree)
{
if (!tree)
return;
struct object *obj = &tree->object;
....
}Рекомендация
Бойтесь неопределенного поведения, не надо рисковать. Трудно предсказать, как UB может проявить себя, и даже не надо пытаться предсказывать. Просто избегайте неопределенного поведения, даже если вам кажется, что всё хорошо и правильно работает.
Кто-то может подумать, что вот именно он-то точно знает, как проявляет себя неопределённое поведение. А значит, ему можно делать то, что нельзя другим, и его код будет работать. Это не так. Чтобы закрепить ощущение опасности, в следующем разделе я продолжу обсуждение неопределённого поведения.
В этот раз я затрудняюсь привести пример из реального приложения. Я регулярно вижу в открытых проектах подозрительный код, который может привести к проблеме, рассмотренной ниже. Но ошибка возможна только при работе с большими массивами, а я не знаю о чужой программе, может ли тот или иной массив быть столь огромен. Поэтому мы обычно не выписываем примеры ошибок, проявляющие себя в 64-битных программах.
Рассмотрим синтетический пример кода:
size_t Count = 1024*1024*1024; // 1 Gb
if (is64bit)
Count *= 5; // 5 Gb
char *array = (char *)malloc(Count);
memset(array, 0, Count);
int index = 0;
for (size_t i = 0; i != Count; i++)
array[index++] = char(i) | 1;
if (array[Count - 1] == 0)
printf("The last array element contains 0.\n");
free(array);Разъяснение
Этот код корректно работает, если собрать 32-битную версию программы; если собрать 64-битный вариант программы, то всё намного интересней.
64-битная программа выделяет массив байт размеров в 5 гигабайт и заполняет его нулями. Затем в цикле массив заполняется какими-то случайными числами, неравными нулю. Чтобы числа не были равны 0, используется "| 1".
Попробуйте угадать, как поведёт себя эта программа, собранная в режиме x64 с помощью компилятора, входящего в состав Visual Studio 2015. Заготовили ответ? Если да, то продолжим.
Если вы запустите отладочную версию этой программы, то она упадёт из-за выхода за границу массива. В какой-то момент переменная index переполнится и её значение станет равно −2147483648 (INT_MIN).
Логичное объяснение? Ничего подобного! Это неопределённое поведение и произойти может всё что угодно.
Для желающих вникнуть в суть, предлагаю следующие ссылки:
Когда я или кто-то ещё говорит, что это неопределённое поведение, люди начинают ворчать. Я не знаю почему, но люди уверены, что точно знают, как работают вычисления в C/C++ и как ведут себя компиляторы.
Но на самом деле они этого не знают. Если бы знали, они бы не говорили всякие глупости. Обычно глупости выглядят как-то так (собирательный образ):
Вы несете теоретический бред. Ну да, формально переполнение 'int' приводит к неопределенному поведению. Но это не более чем болтовня. На практике, всегда можно сказать, что получится. Если к INT_MAX прибавить 1, мы получим INT_MIN. Быть может и есть какие-то экзотические архитектуры, где это не так, но мой компилятор Visual C++ / GCC выдаёт корректный результат.
Так вот, сейчас я без всякой магии на простом примере продемонстрирую неопределённое поведение и не на какой-то волшебной архитектуре, а в Win64-программе.
Достаточно собрать приведённый выше пример в режиме Release x64 и запустить его. Программа перестанет падать, а сообщение "the last array element contains 0" выдано не будет.
Неопределенное поведение здесь проявило себя следующим образом: массив будет полностью заполнен, не смотря, на то, что тип int недостаточен для индексации всех элементов массива. Для тех, кто не верит, предлагаю взглянуть на ассемблерный код:
int index = 0;
for (size_t i = 0; i != Count; i++)
000000013F6D102D xor ecx,ecx
000000013F6D102F nop
array[index++] = char(i) | 1;
000000013F6D1030 movzx edx,cl
000000013F6D1033 or dl,1
000000013F6D1036 mov byte ptr [rcx+rbx],dl
000000013F6D1039 inc rcx
000000013F6D103C cmp rcx,rdi
000000013F6D103F jne main+30h (013F6D1030h)Вот оно проявление неопределенного поведения! И никаких экзотических компиляторов. Это VS2015.
Если заменить int на unsigned неопределённое поведение исчезнет. Массив будет заполнен только частично и в конце будет выдано сообщение "the last array element contains 0".
Ассемблерный код, когда используется unsigned:
unsigned index = 0;
000000013F07102D xor r9d,r9d
for (size_t i = 0; i != Count; i++)
000000013F071030 mov ecx,r9d
000000013F071033 nop dword ptr [rax]
000000013F071037 nop word ptr [rax+rax]
array[index++] = char(i) | 1;
000000013F071040 movzx r8d,cl
000000013F071044 mov edx,r9d
000000013F071047 or r8b,1
000000013F07104B inc r9d
000000013F07104E inc rcx
000000013F071051 mov byte ptr [rdx+rbx],r8b
000000013F071055 cmp rcx,rdi
000000013F071058 jne main+40h (013F071040h)Корректный код
Чтобы всё работало хорошо, надо использовать подходящие типы данных. Если вы собираетесь обрабатывать большие массивы, то забудьте про int и unsigned. Для этого есть типы ptrdiff_t, intptr_t, size_t, DWORD_PTR, std::vector::size_type и так далее. В данном случае пусть будет size_t:
size_t index = 0;
for (size_t i = 0; i != Count; i++)
array[index++] = char(i) | 1;Рекомендация
Если конструкция языка C/С++ вызывает неопределённое поведение, то она его вызывает; и не стоит с этим спорить или предсказывать как оно проявит себя - просто не пишите опасный код.
Есть масса упрямых программистов, которая не хочет видеть ничего опасного в сдвигах отрицательных чисел, переполнении знаковых чисел, сравнивании this с нулём и так далее.
Не будьте в их числе. То, что программа сейчас работает, ещё ничего не значит; как проявит себя UB предсказать невозможно. Ожидаемое поведение программы - это всего лишь один из вариантов UB.
Фрагмент взят из проекта Appleseed. Код содержит ошибку, которую анализатор PVS-Studio диагностирует следующим образом: V719 The switch statement does not cover all values of the 'InputFormat' enum: InputFormatEntity.
enum InputFormat
{
InputFormatScalar,
InputFormatSpectralReflectance,
InputFormatSpectralIlluminance,
InputFormatSpectralReflectanceWithAlpha,
InputFormatSpectralIlluminanceWithAlpha,
InputFormatEntity
};
switch (m_format)
{
case InputFormatScalar:
....
case InputFormatSpectralReflectance:
case InputFormatSpectralIlluminance:
....
case InputFormatSpectralReflectanceWithAlpha:
case InputFormatSpectralIlluminanceWithAlpha:
....
}Разъяснение
Нередко бывает необходимо добавить новую именованную константу в перечисление (enum). Делать это надо очень аккуратно. Здесь подстерегает распространенный паттерн ошибки: забываем где-то добавить case внутрь switch или поправить цепочку операторов if. Подобную ситуацию можно наблюдать в приведённом коде.
В какой-то момент в перечисление InputFormat была добавлена константа InputFormatEntity. Это предположение я делаю на основании того, что эта константа находится в конце. Как правило, программисты добавляют новые константы именно в конец enum.
А вот оператор switch исправить забыли. В результате случай, когда "m_format==InputFormatEntity" никак не обрабатывается.
Корректный код
switch (m_format)
{
case InputFormatScalar:
....
case InputFormatSpectralReflectance:
case InputFormatSpectralIlluminance:
....
case InputFormatSpectralReflectanceWithAlpha:
case InputFormatSpectralIlluminanceWithAlpha:
....
case InputFormatEntity:
....
}Рекомендация
Давайте, подумаем, как можно предотвратить такие ошибки, производя рефакторинг кода.
Самое простое, но не очень удачное решение, это делать везде "default:", который будет уведомлять об ошибке. Например, так:
switch (m_format)
{
case InputFormatScalar:
....
....
default:
assert(false);
throw "В этом месте рассмотрены не все варианты";
}Теперь если переменная m_format будет равна InputFormatEntity, мы заметим ошибку. У этого подхода есть два больших минуса:
1. Ошибку мы можем заметить только на этапе выполнения программы. При чем есть опасность, что ошибка не будет обнаружена на этапе тестирования. Если при выполнении тестов m_format всегда неравна InputFormatEntity, то эта ошибка попадёт в релизную версию продукта. Будет неприятно, когда пользователи начнут сообщать о проблемах.
2. Раз мы считаем, что попасть в default это ошибка, то придётся писать case для всех именованных констант. Это неудобно, особенно если в перечислении этих констант много. Иногда действительно удобно обрабатывать многие ситуации одинаково в ветке default.
Я предлагаю решать эту проблему организационным образом. Он тоже не идеален, но хоть что-то.
Когда в коде вы проверяете значения переменной типа enum, оставляйте комментарий специального вида. Можно использовать какое-то ключевое слово и имя перечисления. Пример:
enum InputFormat
{
InputFormatScalar,
....
InputFormatEntity
//Если добавляешь новую константу, ищи ENUM:InputFormat.
};
switch (m_format) //ENUM:InputFormat
{
....
}Теперь, когда Вы будете изменять enum, вы должны поискать "ENUM:InputFormat". Это даёт гарантию, что вы не пропустите какое-то важное место.
Обговорите это со всеми коллегами, так как каждый должен начать использовать этот комментарий. Если кто-то будет забывать их писать, то всё развалится. Внесите правило по написанию комментариев в стандарт кодирования.
Я думаю вы устали от бесконечного разбора паттернов программистских ошибок. Немного отдохнём и отвлечемся
Итак, типовая ситуация - ваша программа работает неправильно. Но вы не можете дать объяснение происходящему.
В таких ситуациях я всегда призываю не спешить перекладывать вину на кого-то ещё, а сосредоточиться на коде вашей программы. В 99.99% случаев причиной неправильно работы программы, является именно ошибка, которую допустил кто-то из разработчиков вашей команды. Причем, часто эта ошибка весьма глупа и банальна. Вот и ищите её!
То, что ошибка, проявляется нерегулярно, ничего не означает. Просто у вас завёлся Heisenbug.
И упаси вас боже, обвинять компилятор, что он неправильно собирает вашу программу. Такое конечно бывает, но крайне редко. Сами же потом будете глупо выглядеть, когда выяснится, что вы просто не умеете правильно обращаться, скажем, с оператором sizeof(). У меня есть интересная заметка в блоге на эту тему: Во всём виноват компилятор.
Но чтобы восстановить истину, я должен сказать, что бывают и исключения. Очень-очень редко виноват не ваш код. Надо знать о существовании такой возможности. Это позволит не поседеть раньше времени.
Продемонстрирую это на примере, который произошел однажды со мной. Благо, у меня остались соответствующие скриншоты.
У меня отказывался правильно вести себя простой тестовый проект, который я готовил для демонстрации работы анализатора Viva64 (это предшественник PVS-Studio).
После долгих разбирательств выяснилось, что сбоит одна из ячеек памяти. Вернее, один единственный бит. На картинке показано, что я, находясь в режиме отладки, записываю в злосчастную ячейку памяти значение 3:
После изменения памяти, отладчик считывает значения для показа в окне. И показывает число 2:
Видите, там 0x02. Хотя я вводил значение 3.
Младший бит всегда равен нулю.
Программа тестирования памяти подтвердила наличие проблемы. Забавно, что компьютер работал стабильно, и никаких проблем с ним не возникало. Замена планки памяти по гарантии позволила наконец моей программе работать правильно.
Мне очень повезло - я имел дело с простой тестовой программой. И все равно я потратил немало времени, чтобы понять, что происходит. До этого я пару часов изучал ассемблерный листинг, пытаясь найти причину странного поведения. Да, да, я винил в тот момент компилятор.
Не представляю, сколько бы я потратил сил и душевного здоровья, если бы это была какая-то настоящая программа. Спасибо провидению, что мне пришлось в тот момент отлаживать именно демонстрационную утилиту!
Рекомендация
Всегда ищите ошибку в своём коде. Не старайтесь переложить ответственность.
Однако, если ошибка вот уже неделю повторяется только на вашем компьютере, это повод заподозрить неладное.
Продолжайте искать ошибку, но когда будете уходить домой, запустите на ночь тест оперативной памяти. Возможно, это простое действие спасёт тысячи ваших нервных клеток от перегорания.
Фрагмент взят из проекта Haiku (преемница операционной системы BeOS). Код содержит ошибку, которую анализатор PVS-Studio диагностирует следующим образом: V696 The 'continue' operator will terminate 'do { ... } while (FALSE)' loop because the condition is always false.
do {
....
if (appType.InitCheck() == B_OK
&& appType.GetAppHint(&hintRef) == B_OK
&& appRef == hintRef)
{
appType.SetAppHint(NULL);
// try again
continue;
}
....
} while (false);Разъяснение
Оператор continue ведет себя внутри конструкции do { } while () не так, как ожидают некоторые программисты. Когда вызывается оператор continue, в любом случае выполняется проверка условия окончания цикла. Попробую пояснить эти тему подробнее.
Предположим, что программист пишет кода вида:
for (int i = 0; i < n; i++)
{
if (blabla(i))
continue;
foo();
}Или такой:
while (i < n)
{
if (blabla(i++))
continue;
foo();
}Программист на интуитивном уровне понимает, что, когда сработает оператор continue, управление будет передано выше. Проверится условие (i < n). Если оно истинно, начнётся очередная итерация цикла.
Когда же программист пишет код:
do
{
if (blabla(i++))
continue;
foo();
} while (i < n);То интуиция его часто подводит. Вверху он не видит условия и ему кажется, что, вызывая оператор continue, он немедленно запускает очередную итерацию цикла. Это не так. Оператор continue в данном случае передаёт управление к while(i < n) и начинается проверка.
Приведёт непонимание как работает continue к ошибке или нет, зависит от везения. Однако, ошибка точно произойдёт, если условие цикла всегда ложно, как в коде, показанном в самом начале.
Программист планировал выполнять определённые действия, начиная новые итерации с помощью оператора continue. Об этом намерении свидетельствует присутствующий в коде комментарий "//try again
". Однако, никакого "again" не будет, так как используется условие (false), после вызова continue цикл будет остановлен.
Фактически получается, что в конструкции do { ... } while (false); оператор continue эквивалентен по своему действию оператору break.
Корректный код
Вариантов написать корректный код много. Один из них - создать вечный цикл. Для его возобновления использовать continue, а для остановки - оператор break.
for (;;) {
....
if (appType.InitCheck() == B_OK
&& appType.GetAppHint(&hintRef) == B_OK
&& appRef == hintRef)
{
appType.SetAppHint(NULL);
// try again
continue;
}
....
break;
};Рекомендация
Всеми способами старайтесь избегать оператор continue внутри do { ... } while (...);. Уж очень эта конструкция обманчива.
Даже если вы отлично знаете, как всё работает, всё равно не используйте этот оператор. Дело в том, что ошибиться можете не только вы, но и ваши коллеги - они могут неправильно прочитать код и в результате неправильно его модифицировать. Никогда не устану упоминать: хороший программист, этот не тот, кто знает и умеет использовать хитрые конструкции языка, а тот, кто пишет простой и понятный код, который легко сопровождать даже новичку.
В новых стандартах языка С++ появилось много нужного и полезного. Есть и то, что я бы не спешил использовать, по крайней мере рьяно. Но есть и такие нововведения, которые нужно взять на вооружение немедленно. Они однозначно сразу приносят пользу.
Одним из таких нововведений является ключевое слово nullptr, которое призвано заменить макрос NULL.
Напомню, NULL в С++ это ни что иное, как просто 0.
Может показаться, что это синтаксический сахар и не более того. Ну какая разница, пишем мы nullptr или NULL? А разница есть! Использование nullptr реально позволяет избежать разнообразных ошибок.
Я продемонстрирую это на примерах.
Представим, что имеется 2 перегруженных функции:
void Foo(int x, int y, const char *name);
void Foo(int x, int y, int ResourceID);Программист может написать следующий вызов:
Foo(1, 2, NULL);И будет уверен, что тем самым вызовет первую функцию. Но это не так. NULL это не что иное, как 0. А ноль как известно имеет тип int. Поэтому будет вызвана вторая, а не первая функция.
Если бы программист использовал nullptr, такой бы ошибки не возникло. В этом случае будет выбрана именно первая функция.
Ещё можно написать вот такой код:
if (unknownError)
throw NULL;На мой взгляд, генерировать исключение, передавая при этом указатель - подозрительно. Тем не менее, иногда так делают, видимо, разработчикам так было нужно. Впрочем, хорошо или плохо так делать, выходит за рамки обсуждения nullptr.
Важно то, что программист решил в случае неизвестной ошибки сгенерировать исключение и "отправить" во внешний мир нулевой указатель.
На самом деле это не указатель, а int. В результате обработка исключения пойдет не так, как ожидал программист.
Код "throw nullptr;" спасает нас от недоразумения, но это вовсе не значит, что я считаю подобный код нормальным и хорошим.
В ряде случаев, если использовать nullptr, некорректный код просто не будет компилироваться.
Предположим, что какая-то WinApi функция возвращает тип HRESULT. Тип HRESULT не имеет ничего общего с указателем. Однако, вполне можно написать бессмысленный код вида:
if (WinApiFoo(a, b, c) != NULL)Этот код скомпилируется, так как NULL это 0 типа int, а HRESULT это тип long. Вполне можно сравнивать значения типа int и long. Если использовать nullptr, то следующий код не скомпилируется:
if (WinApiFoo(a, b, c) != nullptr)Тем самым, ошибка будет сразу замечена и исправлена.
Я думаю, что вы поняли идею. Таких примеров можно приводить много, но всё это синтетические примеры, а это всегда не очень убедительно. Есть ли какие-то реальные примеры? Да, есть. Вот один из них, только он не такой красивый, короткий и простой.
Код взят из проекта MTASA.
В природе существует RtlFillMemory(). Это может быть настоящая функция или просто макрос, но это не важно. Это аналог функции memset(), но местами поменян 2 и 3 аргумент. Вот как может быть объявлен этот макрос:
#define RtlFillMemory(Destination,Length,Fill) \
memset((Destination),(Fill),(Length))Ещё существует FillMemory(), который есть не что иное, как RtlFillMemory():
#define FillMemory RtlFillMemoryДа, всё длинно и сложно. Но что делать, терпите. Вы ведь наверняка хотели увидеть пример реального кода :).
А вот собственно и код, использующий макрос FillMemory.
LPCTSTR __stdcall GetFaultReason ( EXCEPTION_POINTERS * pExPtrs )
{
....
PIMAGEHLP_SYMBOL pSym = (PIMAGEHLP_SYMBOL)&g_stSymbol ;
FillMemory ( pSym , NULL , SYM_BUFF_SIZE ) ;
....
}Это не код, а какая-то бессмысленность. Как минимум здесь перепутан 2 и 3 аргумент, поэтому анализатор PVS-Studio выдаёт 2 предупреждения V575:
Код скомпилировался из-за того, что NULL это 0; в результате заполняется 0 элементов массива.
На самом деле, ошибка не только в этом - NULL здесь вообще не уместен. Функция memset() работает с байтами, поэтому нет смысла просить её заполнить память значениями NULL, это какая-то абракадабра. Правильный код должен быть таким:
FillMemory(pSym, SYM_BUFF_SIZE, 0);Или таким:
ZeroMemory(pSym, SYM_BUFF_SIZE);Но главное не это. Этот бессмысленный код успешно компилируется. Если бы программист написал:
FillMemory(pSym, nullptr, SYM_BUFF_SIZE);Компилятор выдал бы ошибку. Тогда человек понял бы, что поспешил и сделал что-то не то и отнёсся бы к коду внимательней.
Примечание. Я понимаю, что в данном случае NULL не виноват, тем не менее, именно из-за NULL получилось написать неправильный код, который компилируется без предупреждений.
Рекомендация
Начните использовать nullptr и внесите соответствующий пункт в стандарт кодирования вашей компании. Прямо сейчас.
Использование nullptr позволит избегать некоторых глупых ошибок и тем самым немного ускорит процесс разработки приложения.
Фрагмент взят из проекта Miranda NG. Код содержит ошибку, которую анализатор PVS-Studio диагностирует следующим образом: V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '|' operator..
#define MF_BYCOMMAND 0x00000000L
void CMenuBar::updateState(const HMENU hMenu) const
{
....
::CheckMenuItem(hMenu, ID_VIEW_SHOWAVATAR,
MF_BYCOMMAND | dat->bShowAvatar ? MF_CHECKED : MF_UNCHECKED);
....
}Разъяснение
Выше мы рассмотрели множество ситуаций, которые приводят к неправильной работе программ, но я хочу затронуть вот какую интересную тему. Так бывает, что совершенно неправильный код иногда правильно работает. Опытных программистов этим не удивить, но новичкам, изучающим C/C++, думаю будет интересно рассмотреть один из таких примеров. Впрочем, опытным программистам будет тоже не лишним раз напомнить, чтобы они не жадничали расставлять скобки в сложных выражениях.
Итак, надо вызвать функцию CheckMenuItem() с определённым набором флагов.
Если значение переменной bShowAvatar истинно, то нужен флаг MF_BYCOMMAND и MF_CHECKED.
Иначе: MF_BYCOMMAND и MF_UNCHECKED.
Для этого написано выражение с использование злосчастного тернарного оператора:
MF_BYCOMMAND | dat->bShowAvatar ? MF_CHECKED : MF_UNCHECKED
Дело в том, что приоритет оператора | выше приоритета оператора ?: (см. Приоритет операций в языке Си/Си++). В результате имеют место сразу 2 ошибки.
Первая ошибка: изменилось условие. Условием является не переменная "dat->bShowAvatar", а выражение "MF_BYCOMMAND | dat->bShowAvatar".
Вторая ошибка: выбирается только один из двух флагов: MF_CHECKED или MF_UNCHECKED. Флаг MF_BYCOMMAND "потерялся".
При всём при этом, код работает совершенно правильно! Причина - счастливое стечение обстоятельств и везение программиста. Ему повезло в том, что флаг MF_BYCOMMAND равен 0x00000000L.
Так как флаг MF_BYCOMMAND равен 0, он не оказывает никакого воздействия. Опытные программисты уже всё поняли, но для новичков разберу этот момент поподробнее.
Посмотрим в начале на правильное выражение с дополнительными круглыми скобками:
MF_BYCOMMAND | (dat->bShowAvatar ? MF_CHECKED : MF_UNCHECKED)
Подставим вместо макросов числовые значения:
0x00000000L | (dat->bShowAvatar ? 0x00000008L : 0x00000000L)
Если один из операндов оператора | является 0, то выражение можно упростить:
dat->bShowAvatar ? 0x00000008L : 0x00000000L
Теперь рассмотрим некорректный вариант кода:
MF_BYCOMMAND | dat->bShowAvatar ? MF_CHECKED : MF_UNCHECKED
Подставим вместо макросов числовые значения:
0x00000000L | dat->bShowAvatar ? 0x00000008L : 0x00000000L
В подвыражении "0x00000000L | dat->bShowAvatar" один из операндов оператора | является 0. Сократим выражение:
dat->bShowAvatar ? 0x00000008L : 0x00000000L
Как видите, в результате получили одно и то же выражение. Именно поэтому код с ошибкой работает правильно. Вот такие чудеса нам иногда дарит программирование!
Корректный код
Код можно поправить по-разному - можно добавить круглые скобки; можно добавить промежуточную переменную, возможно, неплохим вариантом будет использовать старый добрый оператор if:
if (dat->bShowAvatar)
::CheckMenuItem(hMenu, ID_VIEW_SHOWAVATAR,
MF_BYCOMMAND | MF_CHECKED);
else
::CheckMenuItem(hMenu, ID_VIEW_SHOWAVATAR,
MF_BYCOMMAND | MF_UNCHECKED);Однако, я не настаиваю на таком способе исправить код. Этот код легко прочитать, но он несколько длинноват.
Рекомендация
Рекомендация проста - старайтесь избегать сложных выражений, особенно если в них вам потребовался тернарный оператор и не жалейте круглые скобки.
Как уже говорилось ранее в главе N4, оператор ?: очень опасен. Легко забыть, что он имеет очень низкий приоритет и легко составить некорректное выражение. Как правило, ?: используют там, где хотят уместить побольше операторов в одну строчку кода, не делайте так.
Странно прочитать столько текста, написанного разработчиком статического анализатора кода, и не услышать рекомендации о его использовании. Исправляюсь, вот она.
Фрагмент взят из проекта Haiku (преемница операционной системы BeOS). Код содержит ошибку, которую анализатор PVS-Studio диагностирует следующим образом: V501 There are identical sub-expressions to the left and to the right of the '<' operator: lJack->m_jackType < lJack->m_jackType
int compareTypeAndID(....)
{
....
if (lJack && rJack)
{
if (lJack->m_jackType < lJack->m_jackType)
{
return -1;
}
....
}Разъяснение
Простая опечатка: в правой части вместо rJack случайно вновь написали lJack.
Опечатка то простая, а ситуация сложная. Дело в том, что здесь стиль программирования или другие приемы бессильны. Люди ошибаются, набирая текст программ, и ничего с этим не сделаешь.
Важно подчеркнуть, что это не проблема каких-то конкретных людей или проектов. Всем людям свойственно ошибаться, и это делают даже профессионалы в серьезных проектах. Вместо многих слов, приглашаю заглянуть сюда. Здесь вы можете наблюдать простейшие опечатки вида A == A, в таких проектах как: Notepad++, WinMerge, Chromium, Qt, Clang, OpenCV, TortoiseSVN, LibreOffice, CoreCLR, Unreal Engine 4 и так далее.
Итак, проблема существует, и относится она вовсе не к лабораторным работам студентам. Когда мне кто-то говорит, что подобные ошибки не делают опытные программисты, я даю ему эту ссылку.
Корректный код
if (lJack->m_jackType < rJack->m_jackType)Рекомендация
В начале о подходах, которые бессильны:
Что может помочь:
Сразу скажу, что у каждой методологии есть свои сильные и слабые стороны, поэтому наиболее качественный и надёжный код можно получить только их сочетанием.
Обзоры кода (code review) позволяют выявить множество разнообразнейших ошибок, а заодно улучшить код в плане читаемости. К сожалению, тщательный совместный обзор кода весьма дорог и утомителен, и при том всё равно не даёт гарантию. Очень сложно сохранить внимание и найти опечатку, рассматривая выражения вида:
qreal l = (orig->x1 - orig->x2)*(orig->x1 - orig->x2) +
(orig->y1 - orig->y2)*(orig->y1 - orig->y1) *
(orig->x3 - orig->x4)*(orig->x3 - orig->x4) +
(orig->y3 - orig->y4)*(orig->y3 - orig->y4);Теоретически спасти нас могут юнит тесты, но это только теоретически. На практике нереально проверить все возможные ветвления программы, да и в самих тестах могут быть ошибки.
Статический анализаторы кода - это просто программы, а не искусственный интеллект. Анализатор не замечает многие ошибки и наоборот, часто ругается на корректный код; но при всех этих недостатках это крайне полезный инструмент. Он может выявить множество ошибок на самом раннем этапе.
Статический анализ кода можно рассматривать как более дешёвую альтернативу Code Review. Программа вместо человека быстро изучает код и предлагает более внимательно проверить определённые фрагменты кода.
Естественно, я предлагаю начать использовать разрабатываемый нами анализатор PVS-Studio. Хотя, конечно, свет клином на нём не сошелся, и есть множество других платных и бесплатных инструментов. Например, можно начать знакомство с методологией статического анализа с бесплатного открытого анализатора Cppcheck. Множество инструментов перечислено на странице Wikipedia: List of tools for static code analysis.
Важное:
Попробуйте статические анализаторы кода, вам они понравятся, это очень хорошее гигиеническое средство.
Напоследок рекомендую ещё вот эту статью от Джона Кармака: Статический анализ кода.
Итак, вам понадобилось реализовать в проекте функциональность X. Теоретики разработки программного обеспечения в этот момент говорят, что для этого нужно взять уже существующую библиотеку Y и использовать её для реализации необходимых вам вещей. Собственно, это классический подход в разработке программного обеспечения - повторное использование своих или чужих наработок (сторонних библиотек). Именно этим путём движется большинство программистов.
Однако, теоретики в статьях и книгах, забывают упомянуть, в какой ад превращается поддержка нескольких десятков сторонних библиотек, живущих в вашем проекте, скажем, по прошествии 10 лет.
Я рекомендую всячески сопротивляться добавлению в проект каждой новой библиотеки. Прошу понять меня правильно - я вовсе не говорю, что не надо использовать библиотеки и писать всё самостоятельно. Это просто-напросто глупо. Дело в том, что часто новая библиотека добавляется в проект по прихоти одного разработчика с целью использовать в ней какую-то маленькую "фитюльку". Добавить новую библиотеку несложно, вот только потом всей команде много лет придётся нести груз её поддержки.
Наблюдая за развитием некоторых больших проектов, я могу перечислить ряд проблем из-за наличия большого количества сторонних библиотек. Наверное, я перечислю далеко не все проблемы, но даже следующий список должен побудить вас задуматься:
Ещё раз подчеркну: я не призываю вас отказаться от использования сторонних библиотек. Если в программе вам понадобилось работать с изображениями в формате PNG, то вам надо взять библиотеку LibPNG и не изобретать велосипед.
Но даже работая с PNG, надо остановиться и подумать. А нужна ли библиотека? Какие операции нужно выполнять с изображениями? Быть может, если вся задача сводится к тому, чтобы сохранить какое-то изображение в *.png - файл, можно обойтись системными функциями. Например, если у вас Windows приложение, то вам поможет WIC. А если вы уже используете библиотеку MFC, то вообще не надо усложнять код, ведь есть класс CImagе (см. обсуждение на сайте Stack Overflow). Минус одна библиотека - отлично!
Приведу пример из собственной практики. В процессе разработки анализатора PVS-Studio, в паре диагностик потребовалось применять простые регулярные выражения. Вообще, я убеждён, что регулярным выражениям не место в статическом анализе - это крайне неэффективный подход. Я даже писал статью на эту тему. Однако иногда, в какой-то строке нужно бывает что-то найти с помощью регулярного выражения.
Можно было бы "прикрутить" какую-то из существующих библиотек. Было понятно, что все они будут избыточны, но ведь регулярные выражения все равно нужны и нужно было принять какое-то решение.
Совершенно случайно, именно в тот момент я читал книгу "Beautiful Code" (ISBN 9780596510046). Эта книга о простых и изящных решениях; в ней я повстречал крайне простую реализацию регулярных выражений. Буквально несколько десятков строк. И всё!
Я взял из книги эту реализацию и начал использовать в PVS-Studio. И знаете, что? До сих пор возможностей этой реализации нам хватает. Какие-то сложные регулярные выражения нам просто не нужны.
Итог. Вместо того, чтобы в проекте появилась какая-то дополнительная библиотека, было потрачено около получаса времени на написание нужной функциональности. Было подавлено желание использовать библиотеку "на все случаи жизни". Как оказалось, это было правильное решение. Это подтверждается тем, что в течение нескольких лет эта самая функциональность "на все случаи" не понадобилась.
Этот случай окончательно убедил меня, что надо по возможности искать простые решения. По возможности, отказываясь от библиотек, вы делаете проект более простым.
Возможно, читателям будет интересно узнать, что же это за такой код для поиска по регулярным выражениям. Перепечатаю его из книги - посмотрите, как элегантно. Это код был мной немного изменён при интеграции в PVS-Studio, но его суть не изменилась. Итак, код из книги:
// Формат регулярного выражения.
// c Соответсвует любой букве "с"
// .(точка) Соответсвует любому одному символу
// ^ Соответсвует началу входящей строки
// $ Соответствует концу входящей строки
// * Соответствует появлению предыдущего символа от нуля до
// нескольких раз
int matchhere(char *regexp, char *text);
int matchstar(int c, char *regexp, char *text);
// match: поиск соответствий регулярному выражению по всему тексту
int match(char *regexp, char *text)
{
if (regexp[0] == '^')
return matchhere(regexp+1, text);
do { /* нужно посмотреть даже пустую строку */
if (matchhere(regexp, text))
return 1;
} while (*text++ != '\0');
return 0;
}
// matchhere: поиск соответствий регулярному выражению в начале текста
int matchhere(char *regexp, char *text)
{
if (regexp[0] == '\0')
return 1;
if (regexp[1] == '*')
return matchstar(regexp[0], regexp+2, text);
if (regexp[0] == '$' && regexp[1] == '\0')
return *text == '\0';
if (*text!='\0' && (regexp[0]=='.' || regexp[0]==*text))
return matchhere(regexp+1, text+1);
return 0;
}
// matchstar: поиск регулярного выражения вида с* с начала текста
int matchstar(int c, char *regexp, char *text)
{
do { /* символ * соответствует нулю или
большему количеству появлений */
if (matchhere(regexp, text))
return 1;
} while (*text != '\0' && (*text++ == c || c == '.'));
return 0;
}Да, этот вариант крайне прост. Да, он мало что может, но вот уже много лет ничего боле сложного не понадобилось, и думаю, не понадобится. Это хороший пример, когда простое решение оказалось лучше полноценного.
Рекомендация
Сопротивляйтесь добавлению в проект новых библиотек. Добавлять следует только тогда, когда очевидно, что без библиотеки не обойтись.
Вот некоторые возможные манёвры:
P.S. Многим рассказанное здесь придется не по душе. Например, то, что я рекомендую использовать не переносимую универсальную библиотеку, а допустим WinAPI. На это будут возражения, основанные на том, что тем самым мы привязываем проект к одной операционной системе. И потом будет очень сложно сделать программу переносимой. Я с этим не согласен. Часто идея "потом перенесем на другу операционную систему" живет только в голове разработчика. На самом деле такая задача вообще может быть никогда не поставлена руководством. Или проект "загнётся" из-за излишней сложности и универсальности, ещё до момента популярности и необходимости портирования. Плюс не забывайте пункт (8) в списке проблем, приведенный выше.
Фрагмент взят из проекта WinMerge. Код содержит ошибку, которую анализатор PVS-Studio диагностирует следующим образом: V530 The return value of function 'empty' is required to be utilized.
void CDirView::GetItemFileNames(
int sel, String& strLeft, String& strRight) const
{
UINT_PTR diffpos = GetItemKey(sel);
if (diffpos == (UINT_PTR)SPECIAL_ITEM_POS)
{
strLeft.empty();
strRight.empty();
}
....
}Разъяснение
Программист хотел очистить строки strLeft и strRight. Строки имеют тип String, который представляет собой не что иное, как std::wstring.
Для очистки он вызвал функцию empty(). Это неправильно - функция empty() не изменяет объект, а только возвращает информацию о том, является строка пустой или нет.
Корректный код
Чтобы исправить ошибку, следует заменить функцию empty() на clear() или erase(). Разработчики WinMerge предпочли erase() и сейчас код выглядит так:
if (diffpos == (UINT_PTR)SPECIAL_ITEM_POS)
{
strLeft.erase();
strRight.erase();
}Рекомендация
Причина подобной ошибки в неудачном имени "empty()". Дело в том, что в разных библиотеках эта функция может обозначать два разных действия.
В одних библиотеках функция empty() очищает объект; в других - возвращает информацию о том, является ли объект пустым.
Слово "empty" плохое. Каждый понимает его по-своему: кто-то считает его "действием", кто-то считает его "запросом информации". Отсюда вся эта путаница и ошибки.
Выход только один - не делайте в своих классах функцию с именем "empty".
Если вы считаете, что это слишком мелкая проблема, то посмотрите сюда. Это весьма распространенный паттерн ошибки. Конечно, менять такие классы как std::string уже поздно, но давайте хотя бы дальше не умножать зло!
Надеюсь, вам понравился этот сборник советов. Конечно, предупредить обо всех способах написать программу неправильно невозможно, да в этом и нет смысла. Моей целью было предостеречь программиста и развить в нем чувство опасности. Возможно, когда программист в очередной раз столкнется с чем-то непонятным, он вспомнит о моих наставлениях и не станет торопиться. Иногда несколько минут изучения документации или написание более простого/ясного кода позволит избежать внесения скрытой ошибки, которая затем несколько лет отравляла бы жизнь пользователям и коллегам.
Пользуясь случаем, приглашаю всех желающих последовать за мной в Twitter: @Code_Analysis.
Желаю всем безбажных программ.
С уважением, Андрей Карпов.
0
0
0
Мы ответим вам на
Если вы так и не получили ответ, пожалуйста, проверьте, отфильтровано ли письмо в одну из следующих стандартных папок:
0