Комбинирането на много действия в един 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.

До C++11:

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

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

От C++11 до C++14:

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

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

От C++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());

може лесно да се срине чрез промяна на нещо друго в програмата. Този код е труден за поддържане. Освен това е неприятно, защото може да ви създаде погрешно впечатление, че всичко работи правилно. Всъщност това е просто набор от обстоятелства и всичко може да се срине при промяна на компилатор или настройки за оптимизация.

Напишете прост код!