Foo(std::move(buffer), line_buffer - buffer.get());

Совмещать много действий в одном выражении языка C++ плохо, так как такой код тяжело понимать, тяжело поддерживать, так в нём еще и легко допустить ошибку. Например, создать баг, совмещая различные действия при вычислении аргументов функции. Мы согласны с классической рекомендацией, что код должен быть прост и понятен. И сейчас рассмотрим интересный случай, когда формально анализатор PVS-Studio не прав, но с практической точки зрения код всё равно стоит изменить.

Порядок вычисления аргументов


То, что будет сейчас рассказано, — это продолжение старой истории о порядке вычисления аргументов, про которую мы писали в статье "Глубина кроличьей норы или собеседование по C++ в компании PVS-Studio".

Краткая суть заключается в следующем. Порядок вычисления аргументов функции — это неуточненное поведение. Стандарт не регламентирует, в каком именно порядке разработчики компиляторов обязаны произвести вычисление аргументов. Например, слева направо (Clang) или справа налево (GCC, MSVC). До стандарта C++17, когда при вычислении аргументов возникали побочные эффекты, это могло приводить к неопределённому поведению.

С появлением стандарта C++17 ситуация изменилась в лучшую сторону: теперь вычисление аргумента и его побочные эффекты начнут выполняться лишь с того момента, как будут выполнены все вычисления и побочные эффекты предыдущего аргумента. Однако, это не значит, что теперь нет места для ошибки.

Рассмотрим простую тестовую программу:

#include <cstdio>
int main()
{
  int i = 1;
  printf("%d, %d\n", i, i++);
  return 0;
}

Что распечатает этот код? Ответ, по-прежнему, зависит от компилятора, его версии и настроения. В зависимости от компилятора может быть распечатано как "1, 1", так и "2, 1". И действительно, воспользовавшись Compiler Explorer я получит следующие результаты:

  • программа, скомпилированная с помощью Clang 11.0.0, выдаёт "1, 1".
  • программа, скомпилированная с помощью GCC 10.2, выдаёт "2, 1".

В этой программе нет неопределённого поведения, но есть неуточнённое поведение (порядок вычисления аргументов).

Код из проекта CSV Parser


Вернёмся к фрагменту кода из проекта CSV Parser, о котором я упоминал в статье "Проверка коллекции header-only C++ библиотек (awesome-hpp)".

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

std::unique_ptr<char[]> buffer(new char[BUFFER_UPPER_LIMIT]);
....
this->feed_state->feed_buffer.push_back(
    std::make_pair<>(std::move(buffer), line_buffer - buffer.get()));

Предупреждение PVS-Studio: V769 The 'buffer.get()' pointer in the 'line_buffer — buffer.get()' expression equals nullptr. The resulting value is senseless and it should not be used. csv.hpp 4957

На самом деле, мы оба неправы, и никакой ошибки нет. Про нюансы будет дальше, а пока начнём с простого.

Итак, давайте разберёмся, почему опасно писать код следующего вида:

Foo(std::move(buffer), line_buffer - buffer.get());

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

#include <iostream>
#include <memory>   

void Print(std::unique_ptr<char[]> p, ptrdiff_t diff)
{
    std::cout << diff << std::endl;
} 

void Print2(ptrdiff_t diff, std::unique_ptr<char[]> p)
{
    std::cout << diff << std::endl;
} 

int main()
{
    {
        std::unique_ptr<char[]> buffer(new char[100]);
        char *ptr = buffer.get() + 22;
        Print(std::move(buffer), ptr - buffer.get());
    }
    {
        std::unique_ptr<char[]> buffer(new char[100]);
        char *ptr = buffer.get() + 22;
        Print2(ptr - buffer.get(), std::move(buffer));
    }
    return 0;
}

Вновь воспользуемся Compiler Explorer и посмотрим результат работы этой программы, собранной разными компиляторами.

Компилятор Clang 11.0.0. Результат:

23387846
22

Компилятор GCC 10.2. Результат:

22
26640070

Результат ожидаем, и писать так нельзя. О чём, собственно, и предупреждает анализатор PVS-Studio.

На этом бы хотелось поставить точку, но всё немного сложнее. Дело в том, что речь идёт о передаче аргументов по значению, а при инстанцировании шаблона функции std::make_pair всё будет иначе. Продолжим погружаться в нюансы и узнаем, почему PVS-Studio в данном случае неправ.

std::make_pair


Обратимся к сайту cppreference и посмотрим, как менялся шаблон функции std::make_pair.

Until C++11:
template< class T1, class T2 >
std::pair<T1,T2> make_pair( T1 t, T2 u );
Since C++11, until C++14:
template< class T1, class T2 >
std::pair<V1,V2> make_pair( T1&& t, T2&& u );
Since C++14:
template< class T1, class T2 >
constexpr std::pair<V1,V2> make_pair( T1&& t, T2&& u );
Как видите, когда-то давным-давно std::make_pair принимал аргументы по значению. Если бы в те времена существовал std::unique_ptr, то рассмотренный выше код действительно был некорректным. Работал бы ли этот код или нет, зависело от везения. На практике, конечно, такая ситуация бы никогда не возникла, так как std::unique_ptr появился в C++11 как замена std::auto_ptr.

Вернёмся в наше время. Начиная с версии стандарта C++11, конструктор начал использовать семантику перемещения.

Здесь есть тонкий момент в том, что std::move на самом деле ничего не перемещает, а всего-навсего производит преобразование объекта к rvalue-ссылке. Это позволит std::make_pair передать указатель новому std::unique_ptr, оставив nullptr в исходном умном указателе. Но эта передача указателя не произойдет, пока мы не попадём внутрь std::make_pair. К тому времени мы уже вычислим line_buffer — buffer.get(), и всё будет хорошо. То есть, вызов функции buffer.get() не может вернуть nullptr в момент, когда он вычисляется, вне зависимости от того, когда именно это произойдёт.

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

Король умер, да здравствует король!


Мы разобрались, что срабатывание, описанное в статье, оказалось ложным. Спасибо одному нашему читателю, который обратил наше внимание на особенность реализации std::make_pair.

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

Здесь уместно вспомнить статью "False positives are our enemies, but may still be your friends". Публикация не наша, но мы с ней согласны.

Это, пожалуй, тот самый случай. Пусть предупреждение ложное, но оно указывает на место, где лучше провести рефакторинг. Достаточно написать что-то вроде этого:

auto delta = line_buffer - buffer.get();
this->feed_state->feed_buffer.push_back(
  std::make_pair(std::move(buffer), delta));

А можно в данной ситуации сделать код еще лучше, воспользовавшись методом emplace_back:

auto delta = line_buffer - buffer.get();
this->feed_state->feed_buffer.emplace_back(std::move(buffer), delta);

Такой код создаст итоговый объект std::pair в контейнере "по месту", минуя создание временного объекта и его перемещение в контейнер. Кстати, анализатор PVS-Studio, предлагает сделать такую замену, выдавая предупреждение V823 из набора правил по микрооптимизациям кода.

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

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

Заключение


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

Код вида:

Foo(std::move(buffer), line_buffer - buffer.get());

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

Пишите код проще!


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. The Code Analyzer is wrong. Long live the Analyzer!.