Объединение множества действий в одно выражение 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, о котором я упоминал в статье Проверка библиотеки 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 Указатель buffer.get() в выражении line_buffer — buffer.get() равен nullptr. Полученное значение бессмысленно и не должно использоваться. 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.

До С++ 11:

шаблон‹ класс T1, класс T2 ›

std::pair‹T1,T2› make_pair( T1 t, T2 u );

Начиная с С++ 11 до С++ 14:

шаблон‹ класс T1, класс T2 ›

std::pair‹V1,V2› make_pair( T1&& t, T2&& u );

Начиная с С++ 14:

шаблон‹ класс T1, класс 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.

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

Здесь уместно вспомнить статью Ложные срабатывания — наши враги, но могут быть и вашими друзьями. Пост не наш, но мы с ним согласны.

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

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());

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

Пишите простой код!