# Топ 10 ошибок в проектах C\+\+ за 2019 год

Ещё один год стремится к окончанию, поэтому настало время заварить себе кофе и перечитать обзоры ошибок за прошедший год\. Конечно, на это уйдёт много времени, поэтому эта статья и была написана\. Предлагаю взглянуть на наиболее интересные темные места проектов, которые встретились нам в 2019 году в проектах, написанных на C и C\+\+\.

![0700_Top_10_C++_Mistakes_2019_ru/image1.png](https://import.viva64.com/docx/blog/0700_Top_10_C++_Mistakes_2019_ru/image1.png)

## Десятое место: "Какая у нас ОС?"

[V1040](https://pvs-studio.ru/ru/docs/warnings/v1040/) Possible typo in the spelling of a pre\-defined macro name\. The '\_\_MINGW32\_' macro is similar to '\_\_MINGW32\_\_'\. winapi\.h 4112

```cpp
#if !defined(__UNICODE_STRING_DEFINED) && defined(__MINGW32_)
#define __UNICODE_STRING_DEFINED
#endif
```

Здесь была допущена опечатка в имени макроса _\_\_MINGW32_\_ \(MINGW32 объявляет \_\_MINGW32\_\_\)\. В других местах проекта как раз проверка написана правильно:

![0700_Top_10_C++_Mistakes_2019_ru/image3.png](https://import.viva64.com/docx/blog/0700_Top_10_C++_Mistakes_2019_ru/image3.png)

Это, кстати, была не только первая ошибка в статье "[CMake: тот случай, когда проекту непростительно качество его кода](https://pvs-studio.ru/ru/blog/posts/cpp/0658/)", но и вообще первая реальная ошибка, найденная диагностикой [V1040](https://pvs-studio.ru/ru/docs/warnings/v1040/) в реальном открытом проекте \(19 августа 2019\)\.

## Девятое место: "Кто первый?"

[V502](https://pvs-studio.ru/ru/docs/warnings/v502/) Perhaps the '?:' operator works in a different way than it was expected\. The '?:' operator has a lower priority than the '\=\=' operator\. mir\_parser\.cpp 884

```cpp
enum Opcode : uint8 {
  kOpUndef,
  ....
  OP_intrinsiccall,
  OP_intrinsiccallassigned,
  ....
  kOpLast,
};

bool MIRParser::ParseStmtIntrinsiccall(StmtNodePtr &stmt, bool isAssigned) {
  Opcode o = !isAssigned ? (....)
                         : (....);
  auto *intrnCallNode = mod.CurFuncCodeMemPool()->New<IntrinsiccallNode>(....);
  lexer.NextToken();
  if (o == !isAssigned ? OP_intrinsiccall : OP_intrinsiccallassigned) {
    intrnCallNode->SetIntrinsic(GetIntrinsicID(lexer.GetTokenKind()));
  } else {
    intrnCallNode->SetIntrinsic(static_cast<MIRIntrinsicID>(....));
  }
  ....
}
```

Нам интересна следующая часть этого кода:

```cpp
if (o == !isAssigned ? OP_intrinsiccall : OP_intrinsiccallassigned) {
  ....
}
```

Оператор '\=\=' имеет более высокий приоритет, чем тернарный оператор \(?:\)\. Из\-за этого условное выражение вычисляется неправильно\. Написанный код эквивалентен следующему:

```cpp
if ((o == !isAssigned) ? OP_intrinsiccall : OP_intrinsiccallassigned) {
  ....
}
```

А с учётом того, что константы _OP\_intrinsiccall_ и _OP\_intrinsiccallassigned_ имеют ненулевые значения, то это условие всегда возвращает истинное значение\. Тело ветки _else_ является недостижимым кодом\.

Эта ошибка пришла в наш топ из статьи "[Проверка кода компилятора Ark Compiler, недавно открытого компанией Huawei](https://pvs-studio.ru/ru/blog/posts/cpp/0690/)"\.

## Восьмое место: "Опасность побитовых операций"

[V1046](https://pvs-studio.ru/ru/docs/warnings/v1046/) Unsafe usage of the bool' and 'int' types together in the operation '&\='\. GSLMultiRootFinder\.h 175

```cpp
int AddFunction(const ROOT::Math::IMultiGenFunction & func) {
  ROOT::Math::IMultiGenFunction * f = func.Clone();
  if (!f) return 0;
  fFunctions.push_back(f);
  return fFunctions.size();
}

template<class FuncIterator>
bool SetFunctionList( FuncIterator begin, FuncIterator end) {
  bool ret = true;
  for (FuncIterator itr = begin; itr != end; ++itr) {
    const ROOT::Math::IMultiGenFunction * f = *itr;
    ret &= AddFunction(*f);
  }
  return ret;
}
```

Исходя из кода ожидалось, что функция _SetFunctionList _обходит список итераторов\. И если хоть один из них будет невалидным, то возвращаемое значение будет _false_, иначе _true_\.

Однако в реальности функция _SetFunctionList_ может возвращать значение _false_ даже для валидных итераторов\. Разберёмся в ситуации\.** **Функция _AddFunction_ возвращает количество валидных итераторов в списке _fFunctions_\. Т\.е\. при добавлении ненулевых итераторов, размер этого списка будет последовательно увеличиваться: 1, 2, 3, 4 и т\.д\. Вот тут и начнёт проявлять себя ошибка в коде:

```cpp
ret &= AddFunction(*f);
```

Т\.к\. функция возвращает результат типа _int_, а не _bool_, то операция '&\=' с чётными числами будет давать значение _false_\. Ведь младший бит чётных чисел всегда будет равен нулю\. Следовательно, такая неочевидная ошибка будет портить результат функции _SetFunctionsList_ даже для валидных аргументов\.

Если вы внимательно читали код из примера \(а вы же внимательно читали, правда?\), то вы могли обратить внимание, что это код из проекта ROOT\. Мы его, конечно, проверяли: "[Анализ кода ROOT \- фреймворка для анализа данных научных исследований](https://pvs-studio.ru/ru/blog/posts/cpp/0682/)"\.

## Седьмое место: "Путаница в переменных"

[V1001](https://pvs-studio.ru/ru/docs/warnings/v1001/) \[CWE\-563\] The 'Mode' variable is assigned but is not used by the end of the function\. SIModeRegister\.cpp 48

```cpp
struct Status {
  unsigned Mask;
  unsigned Mode;

  Status() : Mask(0), Mode(0){};

  Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
    Mode &= Mask;
  };
  ....
};
```

Очень опасно давать аргументам функций те же самые имена, что и членам класса\. Очень легко запутаться\. Перед нами как раз такой случай\. Это выражение не имеет смысла:

```cpp
Mode &= Mask;
```

Меняется аргумент функции\. И всё\. Этот аргумент больше никак не используется\. Скорее всего, надо было написать так:

```cpp
Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
  this->Mode &= Mask;
};
```

А это ошибка из [LLVM](http://llvm.org/)\. У нас есть традиция время от времени анализировать этот проект\. В этом году у нас также появилась [статья](https://pvs-studio.ru/ru/blog/posts/cpp/0629/) о проверке\.

## Шестое место: "В C\+\+ свои законы"

Следующая ошибка появляется в коде из\-за того, что правила С\+\+ не всегда совпадают с математическими правилами или "здравым смыслом"\. Заметите сами, где в небольшом отрывке кода содержится ошибка?

[V709](https://pvs-studio.ru/ru/docs/warnings/v709/) Suspicious comparison found: 'f0 \=\= f1 \=\= m\_fractureBodies\.size\(\)'\. Remember that 'a \=\= b \=\= c' is not equal to 'a \=\= b && b \=\= c'\. btFractureDynamicsWorld\.cpp 483

```cpp
btAlignedObjectArray<btFractureBody*> m_fractureBodies;

void btFractureDynamicsWorld::fractureCallback()
{
  for (int i = 0; i < numManifolds; i++)
  {
    ....
    int f0 = m_fractureBodies.findLinearSearch(....);
    int f1 = m_fractureBodies.findLinearSearch(....);

    if (f0 == f1 == m_fractureBodies.size())
      continue;
    ....
  }
....
}
```

Казалось бы, условие проверяет, что _f0_ равно _f1_ и равно количеству элементов в _m\_fractureBodies_\. Похоже, что это сравнение должно было проверить, находятся ли _f0_ и _f1_ в конце массива _m\_fractureBodies_, поскольку они содержат найденную методом _findLinearSearch\(\)_ позицию объекта\. Однако на самом деле это выражение превращается в проверку, равны ли _f0_ и _f1_, а затем в проверку, равно ли _m\_fractureBodies\.size\(\)_ результату _f0 \=\= f1_\. В итоге, третий операнд здесь сравнивается с 0 или 1\.

Красивая ошибка\! И, к счастью, достаточно редкая\. Пока мы [встречали](https://pvs-studio.ru/ru/blog/examples/v709/) её только в трёх открытых проектах, и, что интересно, все они были как раз игровыми движками\. Кстати, это не единственная ошибка, которую мы обнаружили в Bullet\. Самые интересные попали в нашу статью "[PVS\-Studio заглянул в движок Red Dead Redemption \- Bullet](https://pvs-studio.ru/ru/blog/posts/cpp/0647/)"\.

## Пятое место: "Что есть конец строки?"

Следующая ошибка легко обнаруживается, если знать об одной тонкости\.

[V739](https://pvs-studio.ru/ru/docs/warnings/v739/) EOF should not be compared with a value of the 'char' type\. The 'ch' should be of the 'int' type\. json\.cpp 762

```cpp
void JsonIn::skip_separator()
{
  signed char ch;
  ....
  if (ch == ',') {
    if( ate_separator ) {
      ....
    }
    ....
  } else if (ch == EOF) {
  ....
}
```

Это одна из тех ошибок, которые бывает сложно заметить, если не знать, что _EOF_ определен как \-1\. Соответственно, если пытаться сравнивать его с переменной типа _signed char_, условие почти всегда оказывается _false_\. Единственное исключение, это если кодом символа будет 0xFF \(255\)\. При сравнении такой символ превратится в \-1 и условие окажется верным\.

В этом топе очень много ошибок, связанных с играми: от движков до открытых игр\. Как вы уже могли догадаться, это место также пришло к нам из этой сферы\. Больше ошибок вы можете посмотреть в статье "[Cataclysm Dark Days Ahead, статический анализ и рогалики](https://pvs-studio.ru/ru/blog/posts/cpp/0628/)"\.

## Четвертое место: "Магия числа Пи"

[V624](https://pvs-studio.ru/ru/docs/warnings/v624/) There is probably a misprint in '3\.141592538' constant\. Consider using the M\_PI constant from <math\.h\>\. PhysicsClientC\_API\.cpp 4109

```cpp
B3_SHARED_API void b3ComputeProjectionMatrixFOV(float fov, ....)
{
  float yScale = 1.0 / tan((3.141592538 / 180.0) * fov / 2);
  ....
}
```



Небольшая опечатка в числе Пи \(3,141592653\.\.\.\), пропущено число "6" на 7\-ой позиции в дробной части\. 

![0700_Top_10_C++_Mistakes_2019_ru/image4.png](https://import.viva64.com/docx/blog/0700_Top_10_C++_Mistakes_2019_ru/image4.png)

Возможно, ошибка в десятимиллионной доле после запятой и не приведет к ощутимым последствиям, но все\-таки стоит пользоваться уже существующими библиотечными константами без опечаток\. Для числа Пи, например, есть константа M\_PI из заголовка math\.h\. 

Эта ошибка из уже знакомой нам по шестому месту статьи "[PVS\-Studio заглянул в движок Red Dead Redemption \- Bullet](https://pvs-studio.ru/ru/blog/posts/cpp/0647/)"\. Если вы ещё не отложили её на потом, то это последний шанс\.

## Небольшое лирическое отступление

Вот мы и приблизились к тройке самых интересных ошибок\. Как вы могли заметить, они отсортированы не по катастрофичности последствий их наличия, а по сложности обнаружения\. Ведь в конечном счёте самое главное преимущество статического анализа над code review в том, что машина не устаёт и ничего не забывает\. :\)

А теперь предлагаю вашему вниманию первую тройку\.

![0700_Top_10_C++_Mistakes_2019_ru/image5.png](https://import.viva64.com/docx/blog/0700_Top_10_C++_Mistakes_2019_ru/image5.png)

## Третье место: "Неуловимое исключение"

[V702](https://pvs-studio.ru/ru/docs/warnings/v702/) Classes should always be derived from std::exception \(and alike\) as 'public' \(no keyword was specified, so compiler defaults it to 'private'\)\. CalcManager CalcException\.h 4

```cpp
class CalcException : std::exception
{
public:
  CalcException(HRESULT hr)
  {
    m_hr = hr;
  }
  HRESULT GetException()
  {
    return m_hr;
  }
private:
  HRESULT m_hr;
};
```

Анализатор обнаружил класс, унаследованный от класса _std::exception_ через модификатор _private_ \(модификатор по умолчанию, если ничего не указано\)\. Проблема такого кода заключается в том, что при попытке поймать общее исключение _std::exception_ исключение типа _CalcException_ будет пропущено\. Такое поведение возникает потому, что приватное наследование исключает неявное преобразование типов\. 

Да, не хотелось бы увидеть падение программы из\-за упущенного модификатора _public\._ Кстати, уверен, что вы точно хоть раз использовали в своей жизни приложение, исходный код которого мы сейчас смотрели\. Это старый добрый стандартный [калькулятор Windows](https://github.com/Microsoft/calculator), который мы также [проверяли](https://pvs-studio.ru/ru/blog/posts/cpp/0615/)\.

## Второе место: "Незакрытые HTML\-теги"

[V735](https://pvs-studio.ru/ru/docs/warnings/v735/) Possibly an incorrect HTML\. The "</body\>" closing tag was encountered, while the "</div\>" tag was expected\. book\.cpp 127

```cpp
static QString makeAlgebraLogBaseConversionPage() {
  return
    BEGIN
    INDEX_LINK
    TITLE(Book::tr("Logarithmic Base Conversion"))
    FORMULA(y = log(x) / log(a), log<sub>a</sub>x = log(x) / log(a))
    END;
}
```

Как это часто бывает с C/C\+\+ кодом \- из исходника ничего не понятно, поэтому обратимся к препроцессированному коду для этого фрагмента:

![0700_Top_10_C++_Mistakes_2019_ru/image6.png](https://import.viva64.com/docx/blog/0700_Top_10_C++_Mistakes_2019_ru/image6.png)

Анализатор обнаружил незакрытый тег _<div\>_\. В этом файле много фрагментов html\-кода и теперь его следует дополнительно проверить разработчикам\.

Удивлены, что мы умеем проверять и такое? Когда я впервые это увидел, то был впечатлён\. Так что мы немножечко анализируем html\-код\. Правда, только в коде C\+\+\. :\)

Это не только второе место, но и второй калькулятор в нашем топе\. Со списком всех ошибок вы можете ознакомится в статье "[По следам калькуляторов: SpeedCrunch](https://pvs-studio.ru/ru/blog/posts/cpp/0618/)"\.

## Первое место: "Неуловимые стандартные функции"

Вот мы и добрались до первого места\. Впечатляюще\-странная проблема, которая прошла code review\.

Попробуйте обнаружить её сами:

```cpp
static int
EatWhitespace (FILE * InFile)
  /* ----------------------------------------------------------------------- **
   * Scan past whitespace (see ctype(3C)) and return the first non-whitespace
   * character, or newline, or EOF.
   *
   *  Input:  InFile  - Input source.
   *
   *  Output: The next non-whitespace character in the input stream.
   *
   *  Notes:  Because the config files use a line-oriented grammar, we
   *          explicitly exclude the newline character from the list of
   *          whitespace characters.
   *        - Note that both EOF (-1) and the nul character ('\0') are
   *          considered end-of-file markers.
   *
   * ----------------------------------------------------------------------- **
   */
{
    int c;

    for (c = getc (InFile); isspace (c) && ('\n' != c); c = getc (InFile))
        ;
    return (c);
}                               /* EatWhitespace */
```

Теперь давайте посмотрим, на что ругается анализатор:

[V560](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always true: \('\\n' \!\= c\)\. params\.c 136\.

Странно, не так ли? Давайте посмотрим на кое\-что любопытное в этом же проекте, но в другом файле \(charset\.h\):

```cpp
#ifdef isspace
#undef isspace
#endif
....
#define isspace(c) ((c)==' ' || (c) == '\t')
```

Так, а это уже действительно странно\.\.\. Выходит, если переменная _c _равняется _'\\n', _то совершенно безобидная на первый взгляд функция _isspace\(c\) _вернёт ложь и вторая часть этой проверки не будет выполнена из\-за short\-circuit evaluation\. Если же _isspace\(c\) _будет выполнена, то переменная _c _равна или _' ' _или _'\\t', _а это явно не равно _'\\n'_\.

Конечно, вы можете сказать, что этот макрос похож на _\#define_ _true_ _false_, и такой код никогда не пройдёт code review\. Однако этот код прошёл ревью и благополучно ждал нас в репозитории проекта\.

Если вам нужен более подробный разбор ошибки, то он есть в статье "[Для тех, кто хочет поиграть в детектива: найди ошибку в функции из Midnight Commander](https://pvs-studio.ru/ru/blog/posts/cpp/0610/)"\.

## Заключение

![0700_Top_10_C++_Mistakes_2019_ru/image7.png](https://import.viva64.com/docx/blog/0700_Top_10_C++_Mistakes_2019_ru/image7.png)

За прошедший год мы нашли много ошибок\. Это были привычные ошибки copy\-paste, ошибки в константах, незакрытые теги и множество других проблем\. Но наш анализатор совершенствуется и [учится](https://pvs-studio.ru/ru/blog/posts/0632/) искать всё больше и больше багов, поэтому это далеко не конец, и новые статьи о проверке проектов будут выходить так же часто, как и раньше\.

Если кто\-то читает наши статьи впервые, то на всякий случай уточню, что все эти ошибки были найдены с помощью статического анализатора кода PVS\-Studio, который мы предлагаем [скачать](https://pvs-studio.ru/ru/pvs-studio/download/) и попробовать\. Анализатор умеет выявлять ошибки в коде программ, написанных на языках: C, C\+\+, C\# и Java\.

Вот мы и дошли до конца\! Если же вы пропустили первые два уровня, то предлагаю не упускать возможность и пройти их вместе с нами: [C\#](https://pvs-studio.ru/ru/blog/posts/csharp/0698/) и [Java](https://pvs-studio.ru/ru/blog/posts/java/0699/)\.