Андрей Карпов

09 Мар 2011

Теги:

#Cpp

Как уменьшить вероятность ошибки на этапе написания кода. Заметка N1

09 Мар 2011

Автор: Андрей Карпов

Введение
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 '\0' value. Probably meant: *str != '\0'. clist_modern modern_skinbutton.cpp 282

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

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

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

while ((*str==' ' || *str=='\t') && *str!='\0') str++;
while ((*endstr==' ' || *endstr=='\t') &&
       *endstr!='\0' && 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");
  }
}

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

#Cpp