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

Удаление элементов из вектора с применением цикла

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

int main()
{
   std::vector<int> data{ 1,1,2,3,5,8,13,21,34,55 };

   for (auto it = data.begin(); it != data.end();)
   {
      /* делаем что-то с *it */

      if (*it % 2 == 0)
      {
         data.erase(it);
      }
      else
      {
         ++it;
      }
   }

   for (auto const& e : data)
      std::cout << e << '\n';
}

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

Мы используем std::vector::erase для удаления элемента. Таким образом удаляется элемент, занимающий указанную позицию, и инвалидируются все итераторы и ссылки в точке удаления или после неё, включая итератор end(). При удалении элемента итератор не приращивается, поэтому в следующем цикле будет обрабатываться только что удаленный элемент. А это добром не кончится.

Решение довольно простое: присвойте переменной it значение, возвращаемое функцией erase() (это итератор, следующий за удалённым элементом).

it = data.erase(it);

Это и есть решение. Но есть ли решение получше? Да, мы можем использовать общий алгоритм для удаления всех нужных нам элементов. Для этого мы воспользуемся std::remove_if. Можно заменить цикл for, описанный выше, на следующий:

data.erase(std::remove_if(data.begin(), data.end(), [](int const n) {return n % 2 == 0; }),
           data.end());

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

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

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

void Process(int* ptr)
{
   std::cout << *ptr << '\n';
   delete ptr;
}

void Demo(int const limit)
{
   int* ptr = new int(rand() % 100);

   if (*ptr > limit)
   {
      return;
   }

   Process(ptr);
}

Это может показаться глупым, но представьте, что вместо int есть пользовательский класс, и предусмотрено несколько проверок объекта (или, возможно, связанных с ним данных). Причём, в некоторых случаях выполнение функции должно прекратиться. В противном случае выделенный объект передаётся функции (которая, возможно, получает его во владение). Хватит малейшего недосмотра (особенно если это более длинная и сложная функция), чтобы забыть удалить выделенную память перед возвратом из функции.

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

void Demo(int const limit)
{
   std::unique_ptr<int> ptr = std::make_unique<int>(rand() % 100);

   if (*ptr > limit)
   {
      return;
   }

   Process(ptr.release());
}

Если вам нужно передать основной объект в функцию, которая принимает необработанный указатель и получает объект во владение, просто используйте std::unique_ptr::release(). Это открепит управляемый объект от умного указателя, передавая владение объектом вызывающей функции, именно той, которая отвечает за его высвобождение.

И это приводит к следующей проблеме…

Путаница между освобождением и сбросом

Класс std::unique_ptr имеет два метода:
• release(): освобождает управляемый объект, передавая владение вызывающей стороне.
• reset(): заменяет управляемый объект на другой (принимает аргумент null в качестве значения по умолчанию); если умный указатель содержит не нулевой указатель на объект, этот объект удаляется.

Следующий фрагмент содержит ошибку:

std::unique_ptr<int> MakeObject(int const limit)
{
   std::unique_ptr<int> ptr = std::make_unique<int>(rand() % 100);

   if (*ptr > limit)
   {
      ptr.release();
   }

   return ptr;
}

Если условие не выполняется, управляемый объект высвобождается. Но на самом деле предполагается его удаление. Это приводит к утечке памяти. В данном случае правильно вызвать reset():

std::unique_ptr<int> MakeObject(int const limit)
{
   std::unique_ptr<int> ptr = std::make_unique<int>(rand() % 100);

   if (*ptr > limit)
   {
      ptr.reset();
   }

   return ptr;
}

Подробнее об утечках памяти

Выше было показано, как можно использовать std::unique_ptr, чтобы избежать утечки объектов, выделенных в куче. Но что если нам нужно выделить и освободить массив объектов? Класс std::unique_ptr поддерживает массивы, но, вместе с тем, имеет такие недостатки: размер массива фиксирован, копирование невозможно, std::unique_ptr<[]> не является контейнером и поэтому не совместим с алгоритмами и концепциями. Но есть контейнеры, такие как std::vector, которые могут быть использованы именно в таких целях.

В качестве примера рассмотрим следующий фрагмент:

void Demo()
{
   DWORD size = 0;
   TCHAR* buffer = nullptr;

   BOOL ret = ::GetUserName(nullptr, &size);
   if(!ret && ::GetLastError() == ERROR_INSUFFICIENT_BUFFER)
      buffer = new TCHAR[size];

   if (buffer)
   {
      ::GetUserName(buffer, &size);

      std::cout << buffer << '\n';
   }
}

Многие API Windows требуют, чтобы в распоряжении был буфер определённого размера. Но их можно вызвать и с нулевым буфером, и они вернут размер, необходимый для буфера, чтобы затем вы могли выделить необходимую память. Именно это и происходит в данном фрагменте, который извлекает и выводит имя текущего пользователя. Но выделенный буфер никогда не освобождается (из-за ошибки). Этого можно надёжно избежать, если использовать std::vector, который позаботится о выделении и освобождении внутрисистемно.

void Demo()
{
   DWORD size = 0;
   std::vector<TCHAR> buffer;

   BOOL ret = ::GetUserName(nullptr, &size);
   if (!ret && ::GetLastError() == ERROR_INSUFFICIENT_BUFFER)
      buffer.resize(size);

   if (!buffer.empty())
   {
      ::GetUserName(buffer.data(), &size);

      std::cout << buffer.data() << '\n';
   }
}

Независимо от того, как возвращается эта функция (нормально или даже из-за исключения), память, выделенная объектом std::vector, в этот момент освобождается.

Количество элементов в массиве

Я много раз встречал этот паттерн в унаследованном коде. Где-то объявляется массив (обычно в глобальной области видимости), а затем используется макрос для определения количества элементов в массиве. Вот небольшой фрагмент:

int AnArray[] = {1, 1, 2, 3, 5, 8};

#define COUNT_OF_ARRAY sizeof(AnArray) / sizeof(int)

int main()
{   
   for (size_t i = 0; i < COUNT_OF_ARRAY; ++i)
   {
      std::cout << AnArray[i] << '\n';
   }
}

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

int AnArray[] = {1, 1, 2, 3, 5, 8};

int main()
{
   for (size_t i = 0; i < sizeof(AnArray); ++i)
   {
      std::cout << AnArray[i] << '\n';
   }
}

Предполагается перебрать все элементы массива, но sizeof(AnArray) возвращает размер, занимаемый массивом в памяти, а не количество его элементов. Это приведет к выходу за границы.

Существует несколько решений этой проблемы. Одно из них — использовать некомпонентную функцию std::size(), позволяющую получить количество элементов в массиве:

int AnArray[] = {1, 1, 2, 3, 5, 8};

int main()
{   
   for (size_t i = 0; i < std::size(AnArray); ++i)
   {
      std::cout << AnArray[i] << '\n';
   }
}

Целесообразнее было бы использовать std::array вместо C-подобных массивов. Это даст множество преимуществ, включая лёгкое получение количества элементов с помощью компонентной функции size() (или некомпонентной std::size()).

std::array<int, 6> AnArray {1, 1, 2, 3, 5, 8};

int main()
{   
   for (size_t i = 0; i < AnArray.size(); ++i)
   {
      std::cout << AnArray[i] << '\n';
   }
}

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