Ревью кода на C++: чек-лист из практики

В C++ ревью — не про табуляцию и порядок инклудов. Компилятор молча пропускает код, который упадёт на проде: висячая ссылка на локальную переменную, дважды освобождённый указатель, целочисленное переполнение при вычислении размера буфера. Ни линтер, ни компилятор с флагами по умолчанию этого не поймают. Замечание «обратите внимание на качество кода» ревьюер может не писать вовсе — оно бесполезно. Ниже — что реально ловить и как это формулировать, чтобы автор понял и исправил. Ориентируюсь на C++17 и C++20, отдельно помечаю, где версия важна.

Порядок разделов — по убыванию цены ошибки. Гонка данных и use-after-free стоят инцидента и ночного дежурства. Лишняя копия вектора стоит миллисекунд. Ревьюер, который начинает с форматирования и заканчивает временем жизни объектов, тратит внимание не туда.

Владение ресурсами: кто освобождает и когда

Первое, что смотрю в diff: появились ли new, delete, сырые указатели-владельцы, ручные fopen/fclose, lock/unlock без обёртки. Каждый такой случай — кандидат на утечку или двойное освобождение при раннем return или исключении.

• Сырой new без немедленной передачи в умный указатель. Формулировка замечания: «Здесь Connection* c = new Connection(), а между созданием и delete есть три return и вызов, который может кинуть. Оберните в std::unique_ptr<Connection> — освобождение станет автоматическим на любом пути выхода».
• delete в деструкторе при наличии копирующего конструктора по умолчанию. Классический двойной free: скопировали объект, оба экземпляра держат один указатель, оба его удаляют. Замечание: «Класс владеет указателем через delete в деструкторе, но копирование не запрещено и не переопределено. При копии — двойное освобождение. Либо = delete на копирование, либо правило пяти, либо unique_ptr».
• Мьютекс без lock_guard/scoped_lock. Ручной mtx.lock() оставит мьютекс захваченным, если между ним и unlock() вылетит исключение. Замечание: «Замените lock/unlock на std::scoped_lock lk(mtx) — при исключении мьютекс освободится в раскрутке стека».

Если в diff есть сырой указатель-владелец, вопрос не «нормально ли это», а «на каком пути выхода он течёт». Такой путь почти всегда находится.

RAII¹ как критерий, а не как лозунг

Проверка простая: для каждого ресурса найдите объект, чей деструктор его освобождает. Если такого объекта нет — ресурс освобождается вручную, значит есть путь, где освобождение не случится. Это работает и для памяти, и для файлов, и для сокетов, и для блокировок.

Время жизни: ссылки и указатели на то, что уже умерло

Самый дорогой класс багов в C++ и самый незаметный на ревью, потому что код компилируется и часто даже проходит тесты — падает под нагрузкой или на другом компиляторе.

• Возврат ссылки или указателя на локальную переменную. const std::string& getName() { std::string s = ...; return s; } — ссылка на уничтоженный объект. Компилятор с -Wreturn-local-addr ловит прямой случай, но не через промежуточную переменную.
• Захват по ссылке в лямбде, которая переживёт захваченное. Лямбда с [&], сохранённая в std::function и вызванная позже (колбэк, поток, таймер), обращается к уже уничтоженным локальным переменным. Замечание: «Лямбда сохраняется в член класса и вызывается из другого потока. Захват [&] ссылается на локальные переменные вызывающей функции — к моменту вызова их нет. Захватывайте по значению или через shared_ptr».
• Висячая ссылка из std::string_view (C++17) и std::span (C++20). Оба не владеют данными. std::string_view sv = getTemporary();, где getTemporary() возвращает по значению, — view на уничтоженный временный объект.
• Инвалидация итераторов и ссылок при модификации контейнера. Сохранили ссылку на элемент std::vector, потом сделали push_back — реаллокация, ссылка висячая. Для vector инвалидируется всё при реаллокации; для std::deque, list, map правила другие. Замечание должно называть контейнер и операцию.

// C++20. Пример висячего span — ревью должно это отметить.
std::span<const int> makeSpan() {
    std::vector<int> local = {1, 2, 3};
    return std::span<const int>(local); // span на уничтоженный вектор
}

// Корректно: время жизни вектора длиннее времени жизни span
void process(const std::vector<int>& data) {
    std::span<const int> view(data); // ок, data живёт у вызывающего
    // работаем с view
}

Копии, которых не должно быть

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

• Приём тяжёлого объекта по значению вместо const&. void f(std::vector<int> v) копирует вектор на каждом вызове. Если функция не забирает владение и не модифицирует локально — const std::vector<int>&.
• Цикл for (auto x : container) по контейнеру нетривиальных объектов. Копирует каждый элемент. Замечание: «auto в range-for копирует каждый std::string. Нужно const auto&, если элементы не меняются».
• Возврат std::move из локальной переменной в return. Мешает NRVO (оптимизации возврата) и превращает потенциальное копирование-элизию в гарантированное перемещение — обычно хуже. return std::move(local) в конце функции — почти всегда лишний move.

Где копия оправдана

Если функция сохраняет аргумент себе (кладёт в контейнер-член), приём по значению плюс std::move внутри — корректный и понятный идиом, не замечание. Отличайте забор владения от чтения.

Целочисленные ловушки

Незнаковое переполнение и знаковое переполнение (UB) — источник уязвимостей и «необъяснимых» падений. На ревью ищите арифметику над размерами и индексами.

• Вычитание беззнаковых. size_t результат a - b при b > a — огромное положительное число. for (size_t i = v.size() - 1; ...) при пустом векторе уходит в бесконечный цикл или out-of-bounds.
• Сравнение знакового с беззнаковым. int i = -1; if (i < v.size()) — i приводится к беззнаковому, условие ложно. Компилятор с -Wsign-compare предупреждает; в C++20 есть std::cmp_less и родственные для безопасного сравнения.
• Умножение для размера буфера без проверки переполнения. malloc(count * sizeof(T)) при большом count переполняется и выделяет меньше, чем ожидалось.

Исключения и гарантии

Вопрос на ревью не «использовать ли исключения», а «в каком состоянии останется объект, если операция кинет посередине».

• Модификация нескольких членов без строгой гарантии. Если функция меняет два поля и между ними может вылететь исключение, объект остаётся в промежуточном состоянии. Идиома copy-and-swap даёт строгую гарантию.
• Исключение из деструктора. Если деструктор кидает во время раскрутки стека — std::terminate. Деструкторы неявно noexcept с C++11; бросающий деструктор — почти всегда ошибка.
• Отсутствие noexcept на move-конструкторе. std::vector при реаллокации не станет перемещать элементы, если их move-конструктор не помечен noexcept — он их скопирует ради строгой гарантии. Замечание: «Пометьте move-конструктор noexcept, иначе vector будет копировать вместо перемещения при росте».

Приоритеты: с чего начинать ревью

Форматирование — последняя строка не случайно. Его должен делать clang-format в CI, а не человек в комментариях. Если ревьюер тратит внимание на пробелы, у него не остаётся его на висячие ссылки.

Чего чек-лист не заменяет

Ревью глазами не находит гонки данных и use-after-free надёжно — их находят инструменты. AddressSanitizer, ThreadSanitizer и UndefinedBehaviorSanitizer в CI ловят то, что человек пропускает. Ревью отвечает на вопрос «правильна ли идея и владение ресурсами», санитайзеры — на «нет ли конкретного UB на этом прогоне». Одно без другого дырявое.

Что настроить после

Чек-лист работает, когда его подпирает автоматика. Минимум для C++-проекта:

• clang-format в pre-commit — убирает форматирование из ревью полностью.
• clang-tidy с наборами bugprone-*, cppcoreguidelines-* — ловит часть перечисленного статически.
• Санитайзеры в CI — отдельный прогон тестов с -fsanitize=address,undefined и отдельный с -fsanitize=thread (их нельзя включать одновременно).
• Флаги предупреждений как ошибки — -Wall -Wextra -Wsign-compare и -Werror хотя бы на CI.

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