Как допускать меньше ошибок в коде

Полностью избежать ошибок нереально, их не делает только тот, кто бездвижно лежит на диване. Все программисты допускают ошибки, без этого никуда. Работая над крупными проектами, особенно в команде, ошибок не избежать. Тем не менее их количество реально сократить, чтобы заниматься доработкой гораздо реже. О способах избегания ошибок в программном коде сегодня и расскажем.

Планирование

Вначале любого проекта нужно всё распланировать, этому процессу следует уделить достаточно времени. Кораблю, бесцельно плывущему по морю, никакой ветер не окажется попутным. Очень важно составлять подробный план с пошаговыми действиями и критериями. Ещё полезно подводить промежуточные и конечные итоги. 

Не нужно игнорировать и составление блочных схем или UML-диаграмм. Если выявить ошибку во время проектирования, её решение обойдётся во много раз дешевле, чем та проблема, что выплыла во время разработки или даже внедрения, тестирования. Намного проще переработать схему, чем переписывать код.

Тестирование

Применяйте TDD – разработку через тестирование. После этапа создания плана сперва следует заняться разработкой модульных тестов для дальнейшего кода. Такой подходит исключит вероятность забыть их сделать. Это, конечно же, не главное. Ключевое преимущество подхода – удастся заранее продумать основные моменты интерфейса для классов. Скорее всего реализация пройдёт значительно проще. Такой метод помогает заострить внимание на общих моментах, не беря во внимание отдельные случаи.

Помимо этого, у людей использующий среду Visual Studio Enterprise есть весьма полезная возможность — Live Testing. Она помогает непосредственно в процессе разработки кода (ещё до компиляции) видеть насколько успешно выполнены тесты. Дополнительно есть возможность изучать построчное покрытие тестами и насколько правильно ведётся работа. Главное – всё это в реальном времени. Невероятно полезная возможность, рекомендую использовать всем, кто работает на C#.

Костыли

Не стоит заниматься разработкой собственных костылей. Подавляющее большинство частых задач имеют удачные решения от других разработчиков. Лучше найти готовое решение, которое уже протестировано и доработано коллегами. В таком коде значительно меньший риск обнаружения ошибок. 

Практически наверняка на разработку костыля с нуля уйдёт много времени, а также появится немало ошибок. Не стоит стесняться, применяйте чужие куски кода и дополнительные библиотеки. Что ещё можно говорить, если повторное применение того же самого кода – одна из 6 парадигм ООП.

Чистый код

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

Идеальное решение – лаконичная запись, правильное название переменных, информативные комментарии. Ещё рекомендую применять принципы SOLID, KISS, YAGNI, DRY. В таком коде одного беглого взгляда достаточно для определения, что в нём будет работать неправильно и где нужны коррективы.

Документация 

Нельзя забывать о документации и GIT (или другой системе управления версиями). Когда есть доступ к запискам и истории разработки, значительно проще понять, как и какой кусок кода должен обрабатываться. Это полезно в случаях, если к разработке могут присоединиться новые программисты. 

Сегодня даже не придётся всё делать вручную, есть возможность автоматизировать процесс. Есть возможность применять особые комментарии к объектам и создавать взаимосвязь между задачами. Для этого можно использовать TFS. Чем лучше у программиста представление о работе, тем меньше ошибок появится.

Улучшайте свои знания

Разработчику нужно постоянно развиваться и расширять знания о тонкостях работы с целевым языком. Следует понимать основные ошибки синтаксиса, в этом помогут современные анализаторы и IDE. Все равно в каждом языке существуют неочевидные особенности, что способны негативно отразиться на скорости работы приложения, а иногда и вовсе ломают логику. Вы обязаны искусно обладать инструментом, которым пользуетесь. Конечно же, это позитивно отразится на качестве кода. 

Проверка кода

Следует взять за правило Code Review. Во время продолжительного выполнения какой-то задачи часто появляется замыливание глаз. Эффект приводит к тому, что уже перестаёшь видеть легко заметные ошибки. Это действительно частое состояние. Чтобы избежать серьёзных проблем, следует давать свой код на вычитку коллегам. Чтение чужого кода поможет получить полезный опыт и позволит не допустить тех же ошибок. Обмен опытом всегда полезен.

Простой лайфхак

Код – это творение, именно такое отношение к нему и должно быть. Это продукт работы, и он должен быть качественным. Могу дать один простой лайфхак, который применялся на одном из прошлых проектов – «состязание без багов». 

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

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

Я добрался до кода широко известного клиента мгновенных сообщений Miranda IM. Вместе с различными плагинами это достаточно большой проект, размер которого составляет около 950 тысяч строк кода на C и C++. И, как в любом солидном проекте с историей развития, в нем имеется немалое количество ошибок и опечаток.

Рассматривая дефекты в различных приложениях, я заметил некоторые закономерности. И сейчас на примере дефектов, найденных в Miranda IM, я попробую сформулировать некоторые рекомендации, которые позволят избежать многих ошибок и опечаток ещё на этапе написания кода.

Для анализа Miranda IM я использовал (да, вы уже угадали) анализатор PVS-Studio 4.14. Код проекта Miranda IM весьма качественен, что подтверждается популярностью этой программы. Я и сам являюсь пользователем этого клиента и не имею к нему претензий в плане качества. Проект собирается в Visual Studio с Warning Level 3 (/W3), а количество комментариев составляет 20% от всего текста программы.

1. Избегайте функции memset, memcpy, ZeroMemory и им аналогичные

Мы начнем с ошибок, возникающих при использовании низкоуровневых функций работы с памятью такими, как memset, memcpy, ZeroMemory и им подобных.

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

1) Обработка больших массивов. То есть там, где оптимизированный алгоритм функции даст выигрыш по сравнению с обработкой элементов в простом цикле.

2) Обработка большого количества маленьких массивов. Также имеет смысл с точки зрения улучшения производительности.

Во всех остальных случаях лучше попробовать обойтись без них. Например, я считаю эти функции излишними в такой программе, как Miranda. Никаких ресурсоёмких алгоритмов или огромных массивов в ней нет. Следовательно, использование функций memset/memcpy проистекает только из удобства написания короткого кода. Однако, эта простота очень обманчива и, сэкономив несколько секунд на написании кода, можно неделями вылавливать нечётко проявляющуюся ошибку порчи памяти. Рассмотрим несколько примеров кода, взятых из проекта Miranda IM.

V512 A call of the ‘memcpy’ function will lead to a buffer overflow or underflow. tabsrmm utils.cpp 1080

typedef struct _textrangew
{
  CHARRANGE chrg;
  LPWSTR lpstrText;
} TEXTRANGEW;

const wchar_t* Utils::extractURLFromRichEdit(...)
{
  ...
  ::CopyMemory(tr.lpstrText, L"mailto:", 7);
  ...
}

Копируем только кусок строки. Ошибка банальна до безобразия, но от этого не перестаёт быть ошибкой. Скорее всего, раньше здесь использовалась строка, состоящая из ‘char’. Затем перешли на Unicode строки, а поменять константу забыли.

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

strncpy(tr.lpstrText, "mailto:", 7);

Тогда при переходе к Unicode строкам, число 7 изменять не надо:

wcsncpy(tr.lpstrText, L"mailto:", 7);

Я не говорю, что это идеальный код. Но он уже намного лучше, чем использование CopyMemory. Рассмотрим другой пример.

V568 It’s odd that the argument of sizeof() operator is the ‘& ImgIndex’ expression. clist_modern modern_extraimage.cpp 302

void ExtraImage_SetAllExtraIcons(HWND hwndList,HANDLE hContact)
{
  ...
  char *(ImgIndex[64]);
  ...
  memset(&ImgIndex,0,sizeof(&ImgIndex));
  ...
}

Здесь хотелось обнулить массив, состоящий из 64-указателей. Но вместо этого мы обнулим только первый элемент. Эта же ошибка, кстати, присутствует ещё раз в другом файле. Скажем спасибо Copy-Paste:

V568 It’s odd that the argument of sizeof() operator is the ‘& ImgIndex’ expression. clist_mw extraimage.c 295

Корректный вариант кода должен выглядеть следующим образом:

memset(&ImgIndex,0,sizeof(ImgIndex));

Кстати, взятие адреса у массива может только дополнительно запутать того, кто читает код. Здесь взятие адреса не имеет никакого эффекта. И код может быть написан так:

memset(ImgIndex,0,sizeof(ImgIndex));

Следующий пример.

V568 It’s odd that the argument of sizeof() operator is the ‘& rowOptTA’ expression. clist_modern modern_rowtemplateopt.cpp 258

static ROWCELL* rowOptTA[100];

void rowOptAddContainer(HWND htree, HTREEITEM hti)
{
  ...
  ZeroMemory(rowOptTA,sizeof(&rowOptTA));
  ...
}

Снова, вместо вычисления размера массива, мы вычисляем размер указателя. Правильным выражением будет «sizeof(rowOptTA)». Для очистки массива я предлагаю следующий вариант такого кода:

const size_t ArraySize = 100;
static ROWCELL* rowOptTA[ArraySize];
...
std::fill(rowOptTA, rowOptTA + ArraySize, nullptr);

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

V568 It’s odd that the argument of sizeof() operator is the ‘& rowOptTA’ expression. clist_modern modern_rowtemplateopt.cpp 308

V568 It’s odd that the argument of sizeof() operator is the ‘& rowOptTA’ expression. clist_modern modern_rowtemplateopt.cpp 438

Вы думаете, что это всё касательно низкоуровневой работы с масивами? Нет, не всё. Смотрите дальше, бойтесь и бейте по рукам любителей написать memset.

V512 A call of the ‘memset’ function will lead to a buffer overflow or underflow. clist_modern modern_image_array.cpp 59

static BOOL ImageArray_Alloc(LP_IMAGE_ARRAY_DATA iad, int size)
{
  ...
  memset(&iad->nodes[iad->nodes_allocated_size], 
    (size_grow - iad->nodes_allocated_size) *
       sizeof(IMAGE_ARRAY_DATA_NODE),
    0);
  ...
}

В этот раз размер копируемых данных вычислен корректно. Вот только перепутан местами второй и третий аргумент. Как следствие, мы заполняем 0 элементов. Корректный вариант:

memset(&iad->nodes[iad->nodes_allocated_size], 0,
  (size_grow - iad->nodes_allocated_size) *
     sizeof(IMAGE_ARRAY_DATA_NODE));

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

Возникает вопрос, а как же обойтись без memset при работе с такими структурами, как OPENFILENAME:

OPENFILENAME x;
memset(&x, 0, sizeof(x));

Очень просто. Создать обнуленную структуру можно так:

OPENFILENAME x = { 0 };

2. Внимательно следите, работаете вы со знаковым или беззнаковым типом

Проблема путаницы между signed и unsigned типами может на первый взгляд показаться надуманной. Но этот вопрос зря недооценивается программистами.

В большинстве случаев, людям не нравится рассматривать предупреждения компилятора, связанные с тем, что переменная типа int сравнивается с переменной типа unsigned. И действительно, чаще всего подобный код совершенно корректен. Программисты отключают такие предупреждения или не обращают на них внимания. Или есть третий вариант, они вписывают явное приведение типа, чтобы заглушить предупреждение компилятора, не вдаваясь в подробности.

Предлагаю больше так не делать и каждый раз анализировать ситуацию, когда знаковый тип встречается с беззнаковым. И, вообще, быть внимательным к тому, какой тип имеет выражение или что возвращает функция. Теперь несколько примеров по рассматриваемой теме.

V547 Expression ‘wParam >= 0’ is always true. Unsigned type value is always >= 0. clist_mw cluiframes.c 3140

В коде программы имеется функция id2pos, которая возвращает значение ‘-1’ в случае ошибки. С этой функцией всё в порядке. В другом месте результат работы функции id2pos используется следующим образом:

typedef UINT_PTR WPARAM; 
static int id2pos(int id);
static int nFramescount=0;

INT_PTR CLUIFrameSetFloat(WPARAM wParam,LPARAM lParam)
{
  ...
  wParam=id2pos(wParam);
  if(wParam>=0&&(int)wParam<nFramescount)
    if (Frames[wParam].floating)
  ...
}

Проблема в том, что переменная wParam имеет беззнаковый тип. Следовательно, условие ‘wParam>=0’ всегда истинно. Если функция id2pos вернет ‘-1’, то условие проверки недопустимых значений не сработает, и мы начнем использовать отрицательный индекс.

Я почти уверен, что вначале был написан следующий код:

if (wParam>=0 && wParam<nFramescount)

Компилятор Visual C++ выдал предупреждение «warning C4018: ‘<‘: signed/unsigned mismatch». Это предупреждение как раз включено на Warning Level 3, с которым и собирается Miranda IM. И в этот момент этому месту было уделено недостаточное внимание. Предупреждение было подавлено явным приведением типа. Но ошибка не исчезла, а только спряталась. Корректным вариантом должен был стать следующий код:

if ((int)wParam>=0 && (int)wParam<nFramescount)

Так что внимание и ещё раз внимание к подобным местам. В Miranda IM я насчитал 33 условия, которые из-за путаницы с signed/unsigned всегда истинны или всегда ложны.

Продолжим. Следующий пример мне особенно нравится. А комментарий, просто прекрасен.

V547 Expression ‘nOldLength < 0’ is always false. Unsigned type value is never < 0. IRC mstring.h 229

void Append( PCXSTR pszSrc, int nLength )
{
  ...
  UINT nOldLength = GetLength();
  if (nOldLength < 0)
  {
    // protects from underflow
    nOldLength = 0;
  }
  ...
}

Думаю, пояснений к этому коду не требуется.

Конечно, в ошибках не всегда виноваты только программисты. Иногда свинью подкладывают и разработчики библиотек (в данном случае WinAPI).

#define SRMSGSET_LIMITNAMESLEN_MIN 0
static INT_PTR CALLBACK DlgProcTabsOptions(...)
{
  ...
  limitLength =
    GetDlgItemInt(hwndDlg, IDC_LIMITNAMESLEN, NULL, TRUE) >=
    SRMSGSET_LIMITNAMESLEN_MIN ?
    GetDlgItemInt(hwndDlg, IDC_LIMITNAMESLEN, NULL, TRUE) :
    SRMSGSET_LIMITNAMESLEN_MIN;
  ...
}

Если не обращать внимания на излишне сложное выражение, то код выглядит корректно. Кстати, это вообще была одна строка. Для удобства я разбил её на несколько. Впрочем, форматирование кода сейчас мы затрагивать не будем.

Проблема в том, что функция GetDlgItemInt() возвращает вовсе не ‘int’, как ожидал программист. Эта функция возвращает UINT. Вот её прототип из файла «WinUser.h»:

WINUSERAPI
UINT
WINAPI
GetDlgItemInt(
    __in HWND hDlg,
    __in int nIDDlgItem,
    __out_opt BOOL *lpTranslated,
    __in BOOL bSigned);

В результате PVS-Studio выдает сообщение:

V547 Expression is always true. Unsigned type value is always >= 0. scriver msgoptions.c 458

И это действительно так. Выражение «GetDlgItemInt(hwndDlg, IDC_LIMITNAMESLEN, NULL, TRUE) >= SRMSGSET_LIMITNAMESLEN_MIN» всегда истинно.

Конкретно в данном случае, возможно, никакой ошибки и не будет. Но смысл моего предупреждения, надеюсь, понятен. Внимательно смотрите, что возвращают вам функции.

3. Избегайте большого количества вычислений в одной строке

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

Чаще всего ошибки возникают там, где программист, с целью создать компактный код, собирает в одну строку сразу несколько действий. При незначительном выигрыше в изящности он существенно увеличивает риск опечатки или того, что не учтёт побочные действия. Рассмотрим пример:

V567 Undefined behavior. The ‘s’ variable is modified while being used twice between sequence points. msn ezxml.c 371

short ezxml_internal_dtd(ezxml_root_t root, char *s, size_t len)
{
  ...
  while (*(n = ++s + strspn(s, EZXML_WS)) && *n != '>') {
  ...
}

Здесь возникает неопределенное поведение (undefined behavior). Этот код может корректно функционировать в течение долгого периода жизни программы. Но нет никакой гарантии, что он также будет вести себя после смены версии компилятора или ключей оптимизации. Компилятор в праве вначале вычислить ‘++s’, а затем взывать функцию ‘strspn(s, EZXML_WS)’. Или, наоборот, вначале он может вызвать функцию, а уже затем увеличить значение переменной ‘s’.

Приведу другой пример, почему не стоит пытаться собрать всё в одну строку. В Miranda IM некоторые ветви исполнения программы отключены/включены с помощью вставок вида ‘&& 0’. Примеры:

if ((1 || altDraw) && ...
if (g_CluiData.bCurrentAlpha==GoalAlpha &&0)
if(checkboxWidth && (subindex==-1 ||1)) {

С приведенными сравнениями всё ясно и они хорошо заметны. А теперь представьте, что вы встречаете фрагмент показанный ниже. Код я отформатировал. В программе это ОДНА строка.

V560 A part of conditional expression is always false: 0. clist_modern modern_clui.cpp 2979

LRESULT CLUI::OnDrawItem( UINT msg, WPARAM wParam, LPARAM lParam )
{
  ...
  DrawState(dis->hDC,NULL,NULL,(LPARAM)hIcon,0,
    dis->rcItem.right+dis->rcItem.left-
    GetSystemMetrics(SM_CXSMICON))/2+dx,
    (dis->rcItem.bottom+dis->rcItem.top-
    GetSystemMetrics(SM_CYSMICON))/2+dx,
    0,0,
    DST_ICON|
    (dis->itemState&ODS_INACTIVE&&FALSE?DSS_DISABLED:DSS_NORMAL));
   ...
}

Если здесь нет ошибки, то всё равно непросто вспомнить и найти в этой строке слово FALSE. Вы его нашли? Согласитесь — непростая задача. А если это ошибка? Тогда вообще нет шансов найти её, просто просматривая код. Подобные выражения очень полезно выносить отдельно. Пример:

UINT uFlags = DST_ICON;
uFlags |= dis->itemState & ODS_INACTIVE && FALSE ?
            DSS_DISABLED : DSS_NORMAL;

А я сам, пожалуй, написал бы это длинным, но более понятным способом:

UINT uFlags;
if (dis->itemState & ODS_INACTIVE && (((FALSE))))
  uFlags = DST_ICON | DSS_DISABLED;
else 
  uFlags = DST_ICON | DSS_NORMAL;

Да, это длиннее, но зато легко читается, а слово FALSE лучше заметно.

4. Выравнивайте в коде всё, что возможно

Выравнивание кода уменьшает вероятность допустить опечатку или ошибки при Copy-Paste. Если ошибка все же будет сделана, то найти её в процессе обзора кода будет в несколько раз проще. Рассмотрим пример кода.

V537 Consider reviewing the correctness of ‘maxX’ item’s usage. clist_modern modern_skinengine.cpp 2898

static BOOL ske_DrawTextEffect(...)
{
  ...
  minX=max(0,minX+mcLeftStart-2);
  minY=max(0,minY+mcTopStart-2);
  maxX=min((int)width,maxX+mcRightEnd-1);
  maxY=min((int)height,maxX+mcBottomEnd-1);
  ...
}

Монолитный фрагмент кода, читать который совершенно не интересно. Отформатируем его:

minX = max(0,           minX + mcLeftStart - 2);
minY = max(0,           minY + mcTopStart  - 2);
maxX = min((int)width,  maxX + mcRightEnd  - 1);
maxY = min((int)height, maxX + mcBottomEnd - 1);

Это не самый показательный пример, но согласитесь, теперь стало намного проще заметить, что два раза используется переменная maxX.

Мою рекомендацию по выравниванию не стоит понимать буквально и везде строить столбцы кода. Во-первых, это требует дополнительного времени при написании и правке кода. Во-вторых, это может привести к другим ошибкам. В следующем примере, как раз желание сделать красивый столбик привело к возникновению ошибки в коде Miranda IM.

V536 Be advised that the utilized constant value is represented by an octal form. Oct: 037, Dec: 31. msn msn_mime.cpp 192

static const struct _tag_cpltbl
{
  unsigned cp;
  const char* mimecp;
} cptbl[] =
{
  {   037, "IBM037" },    // IBM EBCDIC US-Canada 
  {   437, "IBM437" },    // OEM United States 
  {   500, "IBM500" },    // IBM EBCDIC International 
  {   708, "ASMO-708" },  // Arabic (ASMO 708) 
  ...
}

Желая сделать красивую колонку чисел, легко увлечься и вписать в начале ‘0’, сделав константу восьмеричной.

Уточняю рекомендацию. Выравнивайте в коде всё, что возможно. Но не выравнивайте числа, дописывая нули.

5. Не размножайте строку более, чем один раз

Копирование строк при программировании неизбежно. Но можно подстраховать себя, не вставляя строку из буфера обмена сразу несколько раз. В большинстве случаев лучше скопировать строку, затем отредактировать. Вновь скопировать и отредактировать. И так далее. Так трудней забыть что-то изменить в строке или изменить её неправильно. Рассмотрим пример кода:

V525 The code containing the collection of similar blocks. Check items ‘1316’, ‘1319’, ‘1318’, ‘1323’, ‘1323’, ‘1317’, ‘1321’ in lines 954, 955, 956, 957, 958, 959, 960. clist_modern modern_clcopts.cpp 954

static INT_PTR CALLBACK DlgProcTrayOpts(...)
{
  ...
  EnableWindow(GetDlgItem(hwndDlg,IDC_PRIMARYSTATUS),TRUE);
  EnableWindow(GetDlgItem(hwndDlg,IDC_CYCLETIMESPIN),FALSE);
  EnableWindow(GetDlgItem(hwndDlg,IDC_CYCLETIME),FALSE);    
  EnableWindow(GetDlgItem(hwndDlg,IDC_ALWAYSPRIMARY),FALSE);
  EnableWindow(GetDlgItem(hwndDlg,IDC_ALWAYSPRIMARY),FALSE);
  EnableWindow(GetDlgItem(hwndDlg,IDC_CYCLE),FALSE);
  EnableWindow(GetDlgItem(hwndDlg,IDC_MULTITRAY),FALSE);
  ...
}

Скорее всего, никакой настоящей ошибки здесь нет. Просто два раза работаем с элементом IDC_ALWAYSPRIMARY. Тем не менее, ошибиться в подобных блоках из скопированных строк весьма легко.

6. Выставляйте высокий уровень предупреждений у компилятора и используйте статические анализаторы

По поводу многих ошибок невозможно дать рекомендаций, как снизить вероятность их появления в коде. Чаще всего они представляют собой опечатки, которые допускает как новичок, так и самый высококвалифицированный программист.

Тем не менее, многие из этих ошибок можно обнаружить ещё на этапе написания кода. В первую очередь — это предупреждения компилятора. А во вторую — отчеты от ночного запуска статических анализаторов кода.

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

Рассмотрим некоторые примеры ошибок, которые могут быть оперативно выявлены анализаторами исходного кода:

V560 A part of conditional expression is always true: 0x01000. tabsrmm tools.cpp 1023

#define GC_UNICODE 0x01000

DWORD dwFlags;

UINT CreateGCMenu(...)
{
  ...
  if (iIndex == 1 && si->iType != GCW_SERVER &&
      !(si->dwFlags && GC_UNICODE)) {
  ...
}

Допущена опечатка. Вместо оператора ‘&’ используется оператор ‘&&’. Как здесь подстраховаться при написании кода, я не знаю. Корректный вариант условия:

 (si->dwFlags & GC_UNICODE)

Следующий пример.

V528 It is odd that pointer to ‘char’ type is compared with the » value. Probably meant: *str != ». clist_modern modern_skinbutton.cpp 282

V528 It is odd that pointer to ‘char’ type is compared with the » value. Probably meant: *endstr != ». clist_modern modern_skinbutton.cpp 283

static char *_skipblank(char * str)
{
  char * endstr=str+strlen(str);
  while ((*str==' ' || *str=='t') && str!='') str++;
  while ((*endstr==' ' || *endstr=='t') &&
         endstr!='' && endstr<str)
    endstr--;
  ...
}

В этом коде всего лишь забыты две звездочки ‘*’ для разыменования указателей. Результат может быть фатальным. Этот код предрасположен к access violation. Корректный вариант кода:

while ((*str==' ' || *str=='t') && *str!='') str++;
while ((*endstr==' ' || *endstr=='t') &&
       *endstr!='' && endstr<str)
  endstr--;

Здесь вновь сложно дать совет, кроме использования специальных инструментов для проверки кода.

Следующий пример.

V514 Dividing sizeof a pointer ‘sizeof (text)’ by another value. There is a probability of logical error presence. clist_modern modern_cachefuncs.cpp 567

#define SIZEOF(X) (sizeof(X)/sizeof(X[0]))

int Cache_GetLineText(..., LPTSTR text, int text_size, ...)
{
  ...
  tmi.printDateTime(pdnce->hTimeZone, _T("t"), text, SIZEOF(text), 0);
  ...
}

На первый взгляд все хорошо. В функцию передается текст и его длина, посчитанная с помощью макроса SIZEOF. На самом деле макрос следует назвать COUNT_OF, но не в этом дело. Беда в том, что мы пытаемся посчитать количество символов в указателе. Здесь вычисляется «sizeof(LPTSTR) / sizeof(TCHAR)». Человек такие подозрительные места замечает плохо, а вот компилятор и статический анализатор хорошо. Исправленный вариант кода:

tmi.printDateTime(pdnce->hTimeZone, _T("t"), text, text_size, 0);

Следующий пример

V560 A part of conditional expression is always true: 0x29. icqoscar8 fam_03buddy.cpp 632

void CIcqProto::handleUserOffline(BYTE *buf, WORD wLen)
{
  ...
  else if (wTLVType = 0x29 && wTLVLen == sizeof(DWORD))
  ...
}

Здесь уместна рекомендация писать константу в условии на первом месте. Вот такой код просто не скомпилируется:

if (0x29 = wTLVType && sizeof(DWORD) == wTLVLen)

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

Если программист отказывается от такого стиля сравнений, то ему остается или полагаться на компилятор/анализатор, или рисковать.

Кстати, несмотря на то, что про эту ошибку все знают, она не самая редкая. Ещё три примера из Miranda IM, где анализатор PVS-Studio выдал предупреждение V559:

else if (ft->ft_magic = FT_MAGIC_OSCAR)
if (ret=0) {return (0);}
if (Drawing->type=CLCIT_CONTACT)

Анализ кода позволяет также выявить если и не ошибки, то очень подозрительные места в коде. Например, в Miranda IM указатели используют далеко не только как указатели. Если в одних местах кода такие игры выглядят нормально, то в других пугают. Пример кода, который меня крайне настораживает:

V542 Consider inspecting an odd type cast: ‘char *’ to ‘char’. clist_modern modern_toolbar.cpp 586

static void
sttRegisterToolBarButton(..., char * pszButtonName, ...)
{
  ...
  if ((BYTE)pszButtonName)
    tbb.tbbFlags=TBBF_FLEXSIZESEPARATOR;
  else
    tbb.tbbFlags=TBBF_ISSEPARATOR;
  ...
}

Фактически мы проверяем, что адрес строки не кратен 256. Мне не понятно, что на самом деле хотели написать в условии. Возможно, это даже правильно, но сильно сомнительно.

Анализом кода можно найти немало некорректных условий. Пример:

V501 There are identical sub-expressions ‘user->statusMessage’ to the left and to the right of the ‘&&’ operator. jabber jabber_chat.cpp 214

void CJabberProto::GcLogShowInformation(...)
{
  ...
  if (user->statusMessage && user->statusMessage)
  ...
}

И так далее и так далее. Я могу ещё долго приводить другие примеры, но это не имеет смысла. Важно то, что большое количество ошибок можно обнаружить с помощью статического анализа на самых ранних этапах.

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

Статический анализ представляет больший интерес в процессе разработки программы, а не в качестве разовых проверок. Множество ошибок и опечаток находится в процессе тестирования и создания юнит-тестов. Но если часть из этих ошибок можно найти ещё на этапе написания кода, то это будет колоссальный выигрыш времени и сил. Обидно два часа отлаживать программу, чтобы потом заметить лишнюю точку с запятой ‘;’ после оператора ‘for’. Такую ошибку можно часто обезвредить, потратив 10 минут на статический анализ измененных в процессе работы файлов.

Заключение

В этой статье я поделился только некоторыми мыслями по поводу того, как допускать меньше ошибок при программировании на Си++. Зреют и другие мысли, о которых я постараюсь написать в следующих статьях и заметках.

P.S.

Уже стало традицией, что после подобной статьи, кто-то спрашивает, а сообщили ли вы разработчикам программы/библиотеки о найденных ошибках. Заранее отвечу на вопрос, отправили ли мы баг репорт по проекту Miranda IM.

Нет, не отправили. Это слишком ресурсоёмкая задача. В статье показана только малая часть от того, что было найдено. В проекте около ста фрагментов, где стоит поправить код, и более ста, где я просто не знаю, ошибка это или нет. Однако, перевод этой статьи будет отправлен авторам Miranda IM и им будет предложена бесплатная версия анализатора PVS-Studio. Если они проявят интерес, то смогут сами проверить исходный код и исправить то, что сочтут нужным.

Ещё стоит пояснить, почему я часто не берусь судить ошибка тот или иной участок кода или нет. Пример неоднозначного кода:

V523 The ‘then’ statement is equivalent to the ‘else’ statement. scriver msglog.c 695

if ( streamData->isFirst ) {
  if (event->dwFlags & IEEDF_RTL) {
    AppendToBuffer(&buffer, &bufferEnd, &bufferAlloced, "\rtlpar");
  } else {
    AppendToBuffer(&buffer, &bufferEnd, &bufferAlloced, "\ltrpar");
  }
} else {
  if (event->dwFlags & IEEDF_RTL) {
    AppendToBuffer(&buffer, &bufferEnd, &bufferAlloced, "\rtlpar");
  } else {
    AppendToBuffer(&buffer, &bufferEnd, &bufferAlloced, "\ltrpar");
  }
}

Вот два одинаковых фрагмента кода. Возможно, это ошибка. А возможно, сейчас в любой ветке нужен одинаковый набор действий. И код написан специально так, чтобы потом его было просто модифицировать. Здесь необходимо знать программу, чтобы разобраться ошибочно это место или нет.

Перевод статьи
«Quality Begins With Code: 10 Ways to Reduce Software Bugs».

Баги в программах это напасть,
преследующая всех разработчиков,
независимо от уровня их навыков.

Мы знаем, почему баги появляются (путей
много, от беспорядочного и сложного
кода до неадекватного тестирования),
но как уменьшить их число?

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

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

1. Создавайте код, который
можно протестировать

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

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

2. Придерживайтесь простоты

Сложный код обречен иметь баги, а кроме
того, его гораздо труднее тестировать.
Ваш код должен делать только самые
необходимые вещи, все остальное — лишь
помехи.

Старайтесь не делать больших коммитов.
Ваши коммиты должны быть краткими,
маленькими, читаемыми, понятными и легко
тестируемыми. Помните, что чем проще
код, тем меньше багов и тем меньше времени
уходит на их исправление. Удаляйте
ненужный код и ни в коем случае не пишите
сложный код без насущной необходимости.

3. Разбивайте ваш код

Подумайте о возможности разделить
ваш код на независимые модули. Разделив
свой код и распределив его по разным
файлам, вы сможете тестировать каждую
часть независимо от других.

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

4. Не считайте комментарии
лейкопластырем

Комментарии в коде должны быть как
можно более краткими. Не нужно пытаться
с их помощью решить проблему читаемости
кода. Это не только вам не поможет, но и
затруднит работу других разработчиков.

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

В целом, старайтесь добавлять только
комментарии, имеющие реальную ценность.

5. Учитывайте предупреждения
компилятора

Не игнорируйте предупреждения
компилятора. Они указывают на проблемы,
которые могут привести к багам в
коде.

Несмотря на то что некоторые
предупреждения могут не идентифицировать
баги, все равно следует обращать на них
внимание. Таким образом вы сможете
остановить нарастание проблем до того,
как они станут критичными.

6. Тестируйте ваш код регулярно

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

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

7. Не спешите

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

Старайтесь не спешить и применять в
разработке лучшие подходы — так вы
избежите затратных (как в денежном, так
и во временном отношении) ошибок.

8. Внедряйте стандарты
написания кода

Это важно. Эффективные стандарты написания кода позволяют сделать разработку более единообразной. Также они дают четкие указания насчет того, как следует подходить к решению тех или иных проблем. Внедрение этих стандартов позволяет избежать распространенных ловушек, способных привести к багам.

В разных компаниях применяются разные
стандарты написания кода. Какие-то свои
стандарты могут быть даже в отдельных
проектах. Вы как разработчик должны
четко их придерживаться.

9. Используйте существующие
библиотеки

Если вы нашли нужную вам функцию в
готовом виде, протестировали и опробовали
ее, — используйте ее в своем коде. Это
сэкономит вам время и уменьшит риск
появления багов.

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

10. Применяйте метод резиновой
уточки

Метод старомодный, но действенный.
Проверяйте свой код строчка за строчкой
и читайте его вашей воображаемой (или
вполне реальной) резиновой уточке. Это
поможет вам убедиться, что ваш код четок,
краток и легок для понимания любым
разработчиком.

В определенных случаях бывает полезным парное программирование: оно помогает держать под контролем сложность программы. Когда над кодом работают две головы вместо одной, это может снизить возможность появления багов, порожденных сложностью кода.

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

Ставьте качество кода на
первое место

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

Помните о правиле бойскаутов: оставляйте все после себя в лучшем виде, чем оно было до вашего появления. Берясь за кусок кода, делайте все возможное, чтобы улучшить его (с учетом лучших практик). Это не избавит вас от багов полностью, но поможет существенно уменьшить их количество.

• Введение
• 1. Избегайте функции memset, memcpy, ZeroMemory и им аналогичные
• 2. Внимательно следите, работаете вы со знаковым или беззнаковым типом
• 3. Избегайте большого количества вычислений в одной строке
• 4. Выравнивайте в коде всё, что возможно
• 5. Не размножайте строку более, чем один раз
• 6. Выставляйте высокий уровень предупреждений у компилятора и используйте статические анализаторы
• Заключение
• P.S.

Я добрался до кода широко известного клиента мгновенных сообщений Miranda IM. Вместе с различными плагинами это достаточно большой проект, размер которого составляет около 950 тысяч строк кода на C и C++. И, как в любом солидном проекте с историей развития, в нем имеется немалое количество ошибок и опечаток.

Введение

Рассматривая дефекты в различных приложениях, я заметил некоторые закономерности. И сейчас на примере дефектов, найденных в Miranda IM, я попробую сформулировать некоторые рекомендации, которые позволят избежать многих ошибок и опечаток ещё на этапе написания кода.

Для анализа Miranda IM я использовал анализатор PVS-Studio 4.14. Код проекта Miranda IM весьма качественен, что подтверждается популярностью этой программы. Я и сам являюсь пользователем этого клиента и не имею к нему претензий в плане качества. Проект собирается в Visual Studio с Warning Level 3 (/W3), а количество комментариев составляет 20% от всего текста программы.

1. Избегайте функции memset, memcpy, ZeroMemory и им аналогичные

Мы начнем с ошибок, возникающих при использовании низкоуровневых функций работы с памятью такими, как memset, memcpy, ZeroMemory и им подобных.

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

1) Обработка больших массивов. То есть там, где оптимизированный алгоритм функции даст выигрыш по сравнению с обработкой элементов в простом цикле.

2) Обработка большого количества маленьких массивов. Также имеет смысл с точки зрения улучшения производительности.

Во всех остальных случаях лучше попробовать обойтись без них. Например, я считаю эти функции излишними в такой программе, как Miranda. Никаких ресурсоёмких алгоритмов или огромных массивов в ней нет. Следовательно, использование функций memset/memcpy проистекает только из удобства написания короткого кода. Однако, эта простота очень обманчива и, сэкономив несколько секунд на написании кода, можно неделями вылавливать нечётко проявляющуюся ошибку порчи памяти. Рассмотрим несколько примеров кода, взятых из проекта Miranda IM.

V512 A call of the ‘memcpy’ function will lead to a buffer overflow or underflow. tabsrmm utils.cpp 1080

typedef struct _textrangew
{
  CHARRANGE chrg;
  LPWSTR lpstrText;
} TEXTRANGEW;

const wchar_t* Utils::extractURLFromRichEdit(...)
{
  ...
  ::CopyMemory(tr.lpstrText, L"mailto:", 7);
  ...
}

Копируем только кусок строки. Ошибка банальна до безобразия, но от этого не перестаёт быть ошибкой. Скорее всего, раньше здесь использовалась строка, состоящая из ‘char’. Затем перешли на Unicode строки, а поменять константу забыли.

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

strncpy(tr.lpstrText, "mailto:", 7);

Тогда при переходе к Unicode строкам, число 7 изменять не надо:

wcsncpy(tr.lpstrText, L"mailto:", 7);

Я не говорю, что это идеальный код. Но он уже намного лучше, чем использование CopyMemory. Рассмотрим другой пример.

V568 It’s odd that the argument of sizeof() operator is the ‘& ImgIndex’ expression. clist_modern modern_extraimage.cpp 302

void ExtraImage_SetAllExtraIcons(HWND hwndList,HANDLE hContact)
{
  ...
  char *(ImgIndex[64]);
  ...
  memset(&ImgIndex,0,sizeof(&ImgIndex));
  ...
}

Здесь хотелось обнулить массив, состоящий из 64-указателей. Но вместо этого мы обнулим только первый элемент. Эта же ошибка, кстати, присутствует ещё раз в другом файле. Скажем спасибо Copy-Paste:

V568 It’s odd that the argument of sizeof() operator is the ‘& ImgIndex’ expression. clist_mw extraimage.c 295

Корректный вариант кода должен выглядеть следующим образом:

memset(&ImgIndex,0,sizeof(ImgIndex));

Кстати, взятие адреса у массива может только дополнительно запутать того, кто читает код. Здесь взятие адреса не имеет никакого эффекта. И код может быть написан так:

memset(ImgIndex,0,sizeof(ImgIndex));

Следующий пример.

V568 It’s odd that the argument of sizeof() operator is the ‘& rowOptTA’ expression. clist_modern modern_rowtemplateopt.cpp 258

static ROWCELL* rowOptTA[100];

void rowOptAddContainer(HWND htree, HTREEITEM hti)
{
  ...
  ZeroMemory(rowOptTA,sizeof(&rowOptTA));
  ...
}

Снова, вместо вычисления размера массива, мы вычисляем размер указателя. Правильным выражением будет «sizeof(rowOptTA)». Для очистки массива я предлагаю следующий вариант такого кода:

const size_t ArraySize = 100;
static ROWCELL* rowOptTA[ArraySize];
...
std::fill(rowOptTA, rowOptTA + ArraySize, nullptr);

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

V568 It’s odd that the argument of sizeof() operator is the ‘& rowOptTA’ expression. clist_modern modern_rowtemplateopt.cpp 308

V568 It’s odd that the argument of sizeof() operator is the ‘& rowOptTA’ expression. clist_modern modern_rowtemplateopt.cpp 438

Вы думаете, что это всё касательно низкоуровневой работы с масивами? Нет, не всё. Смотрите дальше, бойтесь и бейте по рукам любителей написать memset.

V512 A call of the ‘memset’ function will lead to a buffer overflow or underflow. clist_modern modern_image_array.cpp 59

static BOOL ImageArray_Alloc(LP_IMAGE_ARRAY_DATA iad, int size)
{
  ...
  memset(&iad->nodes[iad->nodes_allocated_size], 
    (size_grow - iad->nodes_allocated_size) *
       sizeof(IMAGE_ARRAY_DATA_NODE),
    0);
  ...
}

В этот раз размер копируемых данных вычислен корректно. Вот только перепутан местами второй и третий аргумент. Как следствие, мы заполняем 0 элементов. Корректный вариант:

memset(&iad->nodes[iad->nodes_allocated_size], 0,
  (size_grow - iad->nodes_allocated_size) *
     sizeof(IMAGE_ARRAY_DATA_NODE));

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

Возникает вопрос, а как же обойтись без memset при работе с такими структурами, как

OPENFILENAME:
OPENFILENAME x;
memset(&x, 0, sizeof(x));

Очень просто. Создать обнуленную структуру можно так:

OPENFILENAME x = { 0 };

2. Внимательно следите, работаете вы со знаковым или беззнаковым типом

Проблема путаницы между signed и unsigned типами может на первый взгляд показаться надуманной. Но этот вопрос зря недооценивается программистами.

В большинстве случаев, людям не нравится рассматривать предупреждения компилятора, связанные с тем, что переменная типа int сравнивается с переменной типа unsigned. И действительно, чаще всего подобный код совершенно корректен. Программисты отключают такие предупреждения или не обращают на них внимания. Или есть третий вариант, они вписывают явное приведение типа, чтобы заглушить предупреждение компилятора, не вдаваясь в подробности.

Предлагаю больше так не делать и каждый раз анализировать ситуацию, когда знаковый тип встречается с беззнаковым. И, вообще, быть внимательным к тому, какой тип имеет выражение или что возвращает функция. Теперь несколько примеров по рассматриваемой теме.

V547 Expression ‘wParam >= 0’ is always true. Unsigned type value is always >= 0. clist_mw cluiframes.c 3140

В коде программы имеется функция id2pos, которая возвращает значение ‘-1’ в случае ошибки. С этой функцией всё в порядке. В другом месте результат работы функции id2pos используется следующим образом:

typedef UINT_PTR WPARAM; 
static int id2pos(int id);
static int nFramescount=0;

INT_PTR CLUIFrameSetFloat(WPARAM wParam,LPARAM lParam)
{
  ...
  wParam=id2pos(wParam);
  if(wParam>=0&&(int)wParam<nFramescount)
    if (Frames[wParam].floating)
  ...
}

Проблема в том, что переменная wParam имеет беззнаковый тип. Следовательно, условие ‘wParam>=0’ всегда истинно. Если функция id2pos вернет ‘-1’, то условие проверки недопустимых значений не сработает, и мы начнем использовать отрицательный индекс.

Я почти уверен, что вначале был написан следующий код:

if (wParam>=0 && wParam<nFramescount)

Компилятор Visual C++ выдал предупреждение «warning C4018: ‘<‘ : signed/unsigned mismatch». Это предупреждение как раз включено на Warning Level 3, с которым и собирается Miranda IM. И в этот момент этому месту было уделено недостаточное внимание. Предупреждение было подавлено явным приведением типа. Но ошибка не исчезла, а только спряталась. Корректным вариантом должен был стать следующий код:

if ((INT_PTR)wParam>=0 && (INT_PTR)wParam<nFramescount)

Так что внимание и ещё раз внимание к подобным местам. В Miranda IM я насчитал 33 условия, которые из-за путаницы с signed/unsigned всегда истинны или всегда ложны.

Продолжим. Следующий пример мне особенно нравится. А комментарий, просто прекрасен.

V547 Expression ‘nOldLength < 0’ is always false. Unsigned type value is never < 0. IRC mstring.h 229

void Append( PCXSTR pszSrc, int nLength )
{
  ...
  UINT nOldLength = GetLength();
  if (nOldLength < 0)
  {
    // protects from underflow
    nOldLength = 0;
  }
  ...
}

Думаю, пояснений к этому коду не требуется.

Конечно, в ошибках не всегда виноваты только программисты. Иногда свинью подкладывают и разработчики библиотек (в данном случае WinAPI).

#define SRMSGSET_LIMITNAMESLEN_MIN 0
static INT_PTR CALLBACK DlgProcTabsOptions(...)
{
  ...
  limitLength =
    GetDlgItemInt(hwndDlg, IDC_LIMITNAMESLEN, NULL, TRUE) >=
    SRMSGSET_LIMITNAMESLEN_MIN ?
    GetDlgItemInt(hwndDlg, IDC_LIMITNAMESLEN, NULL, TRUE) :
    SRMSGSET_LIMITNAMESLEN_MIN;
  ...
}

Если не обращать внимания на излишне сложное выражение, то код выглядит корректно. Кстати, это вообще была одна строка. Для удобства я разбил её на несколько. Впрочем, форматирование кода сейчас мы затрагивать не будем.

Проблема в том, что функция GetDlgItemInt() возвращает вовсе не ‘int’, как ожидал программист. Эта функция возвращает UINT. Вот её прототип из файла «WinUser.h»:

WINUSERAPI
UINT
WINAPI
GetDlgItemInt(
    __in HWND hDlg,
    __in int nIDDlgItem,
    __out_opt BOOL *lpTranslated,
    __in BOOL bSigned);

В результате PVS-Studio выдает сообщение:

V547 Expression is always true. Unsigned type value is always >= 0. scriver msgoptions.c 458

И это действительно так. Выражение «GetDlgItemInt(hwndDlg, IDC_LIMITNAMESLEN, NULL, TRUE) >= SRMSGSET_LIMITNAMESLEN_MIN» всегда истинно.

Конкретно в данном случае, возможно, никакой ошибки и не будет. Но смысл моего предупреждения, надеюсь, понятен. Внимательно смотрите, что возвращают вам функции.

3. Избегайте большого количества вычислений в одной строке

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

Чаще всего ошибки возникают там, где программист, с целью создать компактный код, собирает в одну строку сразу несколько действий. При незначительном выигрыше в изящности он существенно увеличивает риск опечатки или того, что не учтёт побочные действия. Рассмотрим пример:

V567 Undefined behavior. The ‘s’ variable is modified while being used twice between sequence points. msn ezxml.c 371

short ezxml_internal_dtd(ezxml_root_t root, char *s, size_t len)
{
  ...
  while (*(n = ++s + strspn(s, EZXML_WS)) && *n != '>') {
  ...
}

Здесь возникает неопределенное поведение (undefined behavior). Этот код может корректно функционировать в течение долгого периода жизни программы. Но нет никакой гарантии, что он также будет вести себя после смены версии компилятора или ключей оптимизации. Компилятор в праве вначале вычислить ‘++s’, а затем взывать функцию ‘strspn(s, EZXML_WS)’. Или, наоборот, вначале он может вызвать функцию, а уже затем увеличить значение переменной ‘s’.

Приведу другой пример, почему не стоит пытаться собрать всё в одну строку. В Miranda IM некоторые ветви исполнения программы отключены/включены с помощью вставок вида ‘&& 0’. Примеры:

if ((1 || altDraw) && ...
if (g_CluiData.bCurrentAlpha==GoalAlpha &&0)
if(checkboxWidth && (subindex==-1 ||1)) {

С приведенными сравнениями всё ясно и они хорошо заметны. А теперь представьте, что вы встречаете фрагмент показанный ниже. Код я отформатировал. В программе это ОДНА строка.

V560 A part of conditional expression is always false: 0. clist_modern modern_clui.cpp 2979

LRESULT CLUI::OnDrawItem( UINT msg, WPARAM wParam, LPARAM lParam )
{
  ...
  DrawState(dis->hDC,NULL,NULL,(LPARAM)hIcon,0,
    dis->rcItem.right+dis->rcItem.left-
    GetSystemMetrics(SM_CXSMICON))/2+dx,
    (dis->rcItem.bottom+dis->rcItem.top-
    GetSystemMetrics(SM_CYSMICON))/2+dx,
    0,0,
    DST_ICON|
    (dis->itemState&ODS_INACTIVE&&FALSE?DSS_DISABLED:DSS_NORMAL));
   ...
}

Если здесь нет ошибки, то всё равно непросто вспомнить и найти в этой строке слово FALSE. Вы его нашли? Согласитесь — непростая задача. А если это ошибка? Тогда вообще нет шансов найти её, просто просматривая код. Подобные выражения очень полезно выносить отдельно. Пример:

UINT uFlags = DST_ICON;
uFlags |= dis->itemState & ODS_INACTIVE && FALSE ?
            DSS_DISABLED : DSS_NORMAL;

А я сам, пожалуй, написал бы это длинным, но более понятным способом:

UINT uFlags;
if (dis->itemState & ODS_INACTIVE && (((FALSE))))
  uFlags = DST_ICON | DSS_DISABLED;
else 
  uFlags = DST_ICON | DSS_NORMAL;

Да, это длиннее, но зато легко читается, а слово FALSE лучше заметно.

4. Выравнивайте в коде всё, что возможно

Выравнивание кода уменьшает вероятность допустить опечатку или ошибки при Copy-Paste. Если ошибка все же будет сделана, то найти её в процессе обзора кода будет в несколько раз проще. Рассмотрим пример кода.

V537 Consider reviewing the correctness of ‘maxX’ item’s usage. clist_modern modern_skinengine.cpp 2898

static BOOL ske_DrawTextEffect(...)
{
  ...
  minX=max(0,minX+mcLeftStart-2);
  minY=max(0,minY+mcTopStart-2);
  maxX=min((int)width,maxX+mcRightEnd-1);
  maxY=min((int)height,maxX+mcBottomEnd-1);
  ...
}

Монолитный фрагмент кода, читать который совершенно не интересно. Отформатируем его:

minX = max(0,           minX + mcLeftStart - 2);
minY = max(0,           minY + mcTopStart  - 2);
maxX = min((int)width,  maxX + mcRightEnd  - 1);
maxY = min((int)height, maxX + mcBottomEnd - 1);

Это не самый показательный пример, но согласитесь, теперь стало намного проще заметить, что два раза используется переменная maxX.

Мою рекомендацию по выравниванию не стоит понимать буквально и везде строить столбцы кода. Во-первых, это требует дополнительного времени при написании и правке кода. Во-вторых, это может привести к другим ошибкам. В следующем примере, как раз желание сделать красивый столбик привело к возникновению ошибки в коде Miranda IM.

V536 Be advised that the utilized constant value is represented by an octal form. Oct: 037, Dec: 31. msn msn_mime.cpp 192

static const struct _tag_cpltbl
{
  unsigned cp;
  const char* mimecp;
} cptbl[] =
{
  {   037, "IBM037" },    // IBM EBCDIC US-Canada 
  {   437, "IBM437" },    // OEM United States 
  {   500, "IBM500" },    // IBM EBCDIC International 
  {   708, "ASMO-708" },  // Arabic (ASMO 708) 
  ...
}

Желая сделать красивую колонку чисел, легко увлечься и вписать в начале ‘0’, сделав константу восьмеричной.

Уточняю рекомендацию. Выравнивайте в коде всё, что возможно. Но не выравнивайте числа, дописывая нули.

5. Не размножайте строку более, чем один раз

Копирование строк при программировании неизбежно. Но можно подстраховать себя, не вставляя строку из буфера обмена сразу несколько раз. В большинстве случаев лучше скопировать строку, затем отредактировать. Вновь скопировать и отредактировать. И так далее. Так трудней забыть что-то изменить в строке или изменить её неправильно. Рассмотрим пример кода:

V525 The code containing the collection of similar blocks. Check items ‘1316’, ‘1319’, ‘1318’, ‘1323’, ‘1323’, ‘1317’, ‘1321’ in lines 954, 955, 956, 957, 958, 959, 960. clist_modern modern_clcopts.cpp 954

static INT_PTR CALLBACK DlgProcTrayOpts(...)
{
  ...
  EnableWindow(GetDlgItem(hwndDlg,IDC_PRIMARYSTATUS),TRUE);
  EnableWindow(GetDlgItem(hwndDlg,IDC_CYCLETIMESPIN),FALSE);
  EnableWindow(GetDlgItem(hwndDlg,IDC_CYCLETIME),FALSE);    
  EnableWindow(GetDlgItem(hwndDlg,IDC_ALWAYSPRIMARY),FALSE);
  EnableWindow(GetDlgItem(hwndDlg,IDC_ALWAYSPRIMARY),FALSE);
  EnableWindow(GetDlgItem(hwndDlg,IDC_CYCLE),FALSE);
  EnableWindow(GetDlgItem(hwndDlg,IDC_MULTITRAY),FALSE);
  ...
}

Скорее всего, никакой настоящей ошибки здесь нет. Просто два раза работаем с элементом IDC_ALWAYSPRIMARY. Тем не менее, ошибиться в подобных блоках из скопированных строк весьма легко.

6. Выставляйте высокий уровень предупреждений у компилятора и используйте статические анализаторы

По поводу многих ошибок невозможно дать рекомендаций, как снизить вероятность их появления в коде. Чаще всего они представляют собой опечатки, которые допускает как новичок, так и самый высококвалифицированный программист.

Тем не менее, многие из этих ошибок можно обнаружить ещё на этапе написания кода. В первую очередь — это предупреждения компилятора. А во вторую — отчеты от ночного запуска статических анализаторов кода.

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

Рассмотрим некоторые примеры ошибок, которые могут быть оперативно выявлены анализаторами исходного кода:

V560 A part of conditional expression is always true: 0x01000. tabsrmm tools.cpp 1023

#define GC_UNICODE 0x01000

DWORD dwFlags;

UINT CreateGCMenu(...)
{
  ...
  if (iIndex == 1 && si->iType != GCW_SERVER &&
      !(si->dwFlags && GC_UNICODE)) {
  ...
}

Допущена опечатка. Вместо оператора ‘&’ используется оператор ‘&&’. Как здесь подстраховаться при написании кода, я не знаю. Корректный вариант условия:

 (si->dwFlags & GC_UNICODE)

Следующий пример.

V528 It is odd that pointer to ‘char’ type is compared with the » value. Probably meant: *str != ». clist_modern modern_skinbutton.cpp 282

V528 It is odd that pointer to ‘char’ type is compared with the » value. Probably meant: *endstr != ». clist_modern modern_skinbutton.cpp 283

static char *_skipblank(char * str)
{
  char * endstr=str+strlen(str);
  while ((*str==' ' || *str=='t') && str!='') str++;
  while ((*endstr==' ' || *endstr=='t') &&
         endstr!='' && endstr<str)
    endstr--;
  ...
}

В этом коде всего лишь забыты две звездочки ‘*’ для разыменования указателей. Результат может быть фатальным. Этот код предрасположен к access violation. Корректный вариант кода:

while ((*str==' ' || *str=='t') && *str!='') str++;
while ((*endstr==' ' || *endstr=='t') &&
       *endstr!='' && endstr<str)
  endstr--;

Здесь вновь сложно дать совет, кроме использования специальных инструментов для проверки кода.

Следующий пример.

V514 Dividing sizeof a pointer ‘sizeof (text)’ by another value. There is a probability of logical error presence. clist_modern modern_cachefuncs.cpp 567

#define SIZEOF(X) (sizeof(X)/sizeof(X[0]))

int Cache_GetLineText(..., LPTSTR text, int text_size, ...)
{
  ...
  tmi.printDateTime(pdnce->hTimeZone, _T("t"), text, SIZEOF(text), 0);
  ...
}

На первый взгляд все хорошо. В функцию передается текст и его длина, посчитанная с помощью макроса SIZEOF. На самом деле макрос следует назвать COUNT_OF, но не в этом дело. Беда в том, что мы пытаемся посчитать количество символов в указателе. Здесь вычисляется «sizeof(LPTSTR) / sizeof(TCHAR)». Человек такие подозрительные места замечает плохо, а вот компилятор и статический анализатор хорошо. Исправленный вариант кода:

tmi.printDateTime(pdnce->hTimeZone, _T("t"), text, text_size, 0);

Следующий пример

V560 A part of conditional expression is always true: 0x29. icqoscar8 fam_03buddy.cpp 632

void CIcqProto::handleUserOffline(BYTE *buf, WORD wLen)
{
  ...
  else if (wTLVType = 0x29 && wTLVLen == sizeof(DWORD))
  ...
}

Здесь уместна рекомендация писать константу в условии на первом месте. Вот такой код просто не скомпилируется:

if (0x29 = wTLVType && sizeof(DWORD) == wTLVLen)

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

Если программист отказывается от такого стиля сравнений, то ему остается или полагаться на компилятор/анализатор, или рисковать.

Кстати, несмотря на то, что про эту ошибку все знают, она не самая редкая. Ещё три примера из Miranda IM, где анализатор PVS-Studio выдал предупреждение V559:

else if (ft->ft_magic = FT_MAGIC_OSCAR)
if (ret=0) {return (0);}
if (Drawing->type=CLCIT_CONTACT)

Анализ кода позволяет также выявить если и не ошибки, то очень подозрительные места в коде. Например, в Miranda IM указатели используют далеко не только как указатели. Если в одних местах кода такие игры выглядят нормально, то в других пугают. Пример кода, который меня крайне настораживает:

V542 Consider inspecting an odd type cast: ‘char *’ to ‘char’. clist_modern modern_toolbar.cpp 586

static void
sttRegisterToolBarButton(..., char * pszButtonName, ...)
{
  ...
  if ((BYTE)pszButtonName)
    tbb.tbbFlags=TBBF_FLEXSIZESEPARATOR;
  else
    tbb.tbbFlags=TBBF_ISSEPARATOR;
  ...
}

Фактически мы проверяем, что адрес строки не кратен 256. Мне не понятно, что на самом деле хотели написать в условии. Возможно, это даже правильно, но сильно сомнительно.

Анализом кода можно найти немало некорректных условий. Пример:

V501 There are identical sub-expressions ‘user->statusMessage’ to the left and to the right of the ‘&&’ operator. jabber jabber_chat.cpp 214

void CJabberProto::GcLogShowInformation(...)
{
  ...
  if (user->statusMessage && user->statusMessage)
  ...
}

И так далее и так далее. Я могу ещё долго приводить другие примеры, но это не имеет смысла. Важно то, что большое количество ошибок можно обнаружить с помощью статического анализа на самых ранних этапах.

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

Статический анализ представляет больший интерес в процессе разработки программы, а не в качестве разовых проверок. Множество ошибок и опечаток находится в процессе тестирования и создания юнит-тестов. Но если часть из этих ошибок можно найти ещё на этапе написания кода, то это будет колоссальный выигрыш времени и сил. Обидно два часа отлаживать программу, чтобы потом заметить лишнюю точку с запятой ‘;’ после оператора ‘for’. Такую ошибку можно часто обезвредить, потратив 10 минут на статический анализ измененных в процессе работы файлов.

Заключение

В этой статье я поделился только некоторыми мыслями по поводу того, как допускать меньше ошибок при программировании на Си++. Зреют и другие мысли, о которых я постараюсь написать в следующих статьях и заметках.

P.S.

Уже стало традицией, что после подобной статьи, кто-то спрашивает, а сообщили ли вы разработчикам программы/библиотеки о найденных ошибках. Заранее отвечу на вопрос, отправили ли мы баг репорт по проекту Miranda IM.

Нет, не отправили. Это слишком ресурсоёмкая задача. В статье показана только малая часть от того, что было найдено. В проекте около ста фрагментов, где стоит поправить код, и более ста, где я просто не знаю, ошибка это или нет. Однако, перевод этой статьи будет отправлен авторам Miranda IM и им будет предложена бесплатная версия анализатора PVS-Studio. Если они проявят интерес, то смогут сами проверить исходный код и исправить то, что сочтут нужным.

Ещё стоит пояснить, почему я часто не берусь судить ошибка тот или иной участок кода или нет. Пример неоднозначного кода:

V523 The ‘then’ statement is equivalent to the ‘else’ statement. scriver msglog.c 695

if ( streamData->isFirst ) {
  if (event->dwFlags & IEEDF_RTL) {
    AppendToBuffer(&buffer, &bufferEnd, &bufferAlloced, "\rtlpar");
  } else {
    AppendToBuffer(&buffer, &bufferEnd, &bufferAlloced, "\ltrpar");
  }
} else {
  if (event->dwFlags & IEEDF_RTL) {
    AppendToBuffer(&buffer, &bufferEnd, &bufferAlloced, "\rtlpar");
  } else {
    AppendToBuffer(&buffer, &bufferEnd, &bufferAlloced, "\ltrpar");
  }
}

Вот два одинаковых фрагмента кода. Возможно, это ошибка. А возможно, сейчас в любой ветке нужен одинаковый набор действий. И код написан специально так, чтобы потом его было просто модифицировать. Здесь необходимо знать программу, чтобы разобраться ошибочно это место или нет.

import itertools
import random
from typing import List

class FakeObject(object):
   def __init__(self):
       self.red = random.randint(0, 255)
       self.blue = random.randint(0, 255)
       self.green = random.randint(0, 255)
   def __str__(self):
       return f"r: {self.red}, b: {self.blue}, g: {self.green}"
def fake_feature(fakes: List[FakeObject]) -> int:
   red, blue, green = 0, 0, 0
   for index, i_fake in enumerate(fakes):
       red = (red + index * i_fake.red) % 255
       blue = (blue + index * i_fake.blue) % 255
       green = (green + index * i_fake.green) % 255
   return red + blue + green
print(
   dict(
       sorted(
           map(
               lambda x: (fake_feature(x), list(map(str, x))),
               itertools.permutations(
                   filter(
                       lambda x: random.random() > 0.5,
                       map(lambda x: FakeObject(), range(10))
                   )
               )
           ),
           key=lambda x: -x[0]
       )[0:10]
   )
)

Научитесь: Профессия DevOps-инженер
Узнать больше