60 антипаттернов для С++ программиста, часть 8 (совет 36 — 40)

Перед вами обновлённая коллекция вредных советов для C++ программистов, которая превратилась в целую электронную книгу. Всего их 60, и каждый сопровождается пояснением, почему на самом деле ему не стоит следовать. Всё будет одновременно и в шутку, и серьёзно. Как бы глупо ни смотрелся вредный совет, он не выдуман, а подсмотрен в реальном мире программирования.

Я буду публиковать советы по 5 штук, чтобы не утомить вас, так как мини-книга содержит много интересных отсылок на другие статьи, видео и т. д. Однако, если вам не терпится, здесь вы можете сразу перейти к её полному варианту: "60 антипаттернов для С++ программиста". В любом случае желаю приятного чтения.

Вредный совет N36. Добавляй, авось пригодится

Подключайте как можно больше заголовочных файлов, чтобы каждый .cpp файл раскрывался в миллион строк – коллеги скажут спасибо за то, что у них больше времени на перекур во время пересборки!

Воздержусь от совета в стиле "не делайте так" от Капитана Очевидность. Иногда действительно бывают ситуации, когда требуется включать много заголовочных файлов. Что же тогда делать для ускорения сборки проекта? Вам помогут предкомпилируемые заголовочные файлы, которые мы рассматривали в главе N27!

Дополнительно вам могут быть полезны эти публикации моих коллег:

1. Ускорение сборки C и C++ проектов;
2. Ускоряем сборку и анализ при помощи Incredibuild.

Вредный совет N37. Создай свой h-квест

Пишите ваши .h-файлы так, чтобы они зависели от других заголовков, и при этом не включайте их в свой заголовочный файл. Пусть тот, кто инклудит, догадается, какие заголовки нужно заранее заинклудить перед использованием вашего файла. Развлеките коллег квестами!

Давайте поясню на примере, что имеется в виду. Пусть в файле my_types.h объявлены различные ваши типы:

// my_types.h
#ifndef MY_TYPES_H
#define MY_TYPES_H

typedef int result_type;
struct mystruct {
  int member;
};

#endif

В другом заголовочном файле my_functions.h описаны, функции использующие эти типы.

// my_functions.h
#ifndef MY_FUNCTIONS_H
#define MY_FUNCTIONS_H

result_type foo(mystruct &s);
result_type doo(mystruct &s);

#endif

Если в cpp-файле требуется использовать функции, описанные в my_functions.h, то перед ним потребуется включить и файл с описанием типов, иначе код не скомпилируется:

// my.cpp
#include "my_types.h"
#include "my_functions.h"

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

Во-первых, раздражает выяснять, от каких файлов зависит тот или иной заголовочный файл и что нужно перед ним включить.

Во-вторых, такой подход порождает желание включать сразу побольше наиболее часто используемых заголовочных файлов. Как говорится, "про запас". В результате в начале cpp-файлов возникают длинные списки заголовочных файлов, часть из которых на самом деле не нужна и только замедляет компиляцию.

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

Для рассмотренного нами примера следует изменить файлы следующим образом:

// my_types.h
#ifndef MY_TYPES_H
#define MY_TYPES_H

typedef int result_type;
struct mystruct {
  int member;
};
#endif
--------------------------------
// my_functions.h
#ifndef MY_FUNCTIONS_H
#define MY_FUNCTIONS_H

#include "my_types.h"

result_type foo(mystruct &s);
result_type doo(mystruct &s);
#endif
--------------------------------
// my.cpp
#include "my_functions.h"

Вредный совет N38. C-style cast

*Зачем нужны все эти *_cast, если есть reinterpret_cast? А ещё лучше и короче старый добрый C-style cast: (Type)(expr)*.**

Дело в том, что приведения в стиле языка C более "жёсткие". Их использование позволяет выполнить изменение типа, которое не имеет смысла. В случае более мягких *_cast преобразований такой код не скомпилируется, благодаря чему можно было бы обнаружить ошибку ещё на этапе написания кода.

Рассмотрим синтетический фрагмент кода Windows-приложения:

void ff(char *);
void ff(wchar_t *);

void foo(const TCHAR *str)
{
    ff((char *)(str));            // (1) компилируется
    ff(const_cast<char *>(str));  // (2) не компилируется
    ff(const_cast<TCHAR *>(str)); // (3) компилируется
}

Предположим, нужно снять константность с указателя на символы типа TCHAR. Тип TCHAR может раскрываться в зависимости от режима компиляции в char или в wchar_t. Пусть в нашем случае он раскрывается как раз в тип wchar_t.

Рассмотрим три разных приведения типа:

1. Если использовать С-style приведение типа, то не только снимается константность, но и меняется базовый тип указателя. В результате выбирается неправильная функция ff. Код содержит ошибку, но компилируется.
2. Использован оператор const_cast, который должен снять только константность. Поскольку базовый тип указателя не совпадает, то возникает ошибка компиляции. Отлично! Баг выявлен ещё на этапе компиляции.
3. Исправленный код.

Дополнительные ссылки:

1. When should static_cast, dynamic_cast, const_cast, and reinterpret_cast be used?
2. Regular cast vs. static_cast vs. dynamic_cast.
3. Why use static_cast<int>(x) instead of (int)x?
4. What is the difference between static_cast<> and C style casting?
5. Приведение типов в C++.
6. static_cast и (int) — это одно и то же?

Вредный совет N39. Универсальность — это круто

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

Да, функции с переменным количеством аргументов позволяют делать очень много. В том числе допустить множество ошибок при их реализации и использовании :).

Сложно написать надёжный, неподверженный потенциальным уязвимостям код, использующий такие функции. Поэтому некоторые стандарты, такие как, например, SEI CERT C++ Coding Standard, вообще рекомендуют их не использовать: DCL50-CPP. Do not define a C-style variadic function.

И ещё одна ссылка: Ellipsis (and why to avoid them).

Мы ещё вернёмся к этой теме в совете N58, где будем говорить про функцию printf.

Вредный совет N40. Ты властелин указателей — делай что хочешь

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

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

float rgb[3];
float alphaСhannel;
....
for (int i = 0; i < 4; i++)
  rgb[i] = 0f;

Кому-то было лень отдельно записывать ноль в переменную для альфа-канала. Он совместил её инициализацию с инициализацией элементов массива.

Плохой и опасный способ по трём соображениям:

1. Такой код не имеет никаких преимуществ. Сэкономлена одна строка кода на явную инициализацию переменной alphaСhannel, но у меня язык не поворачивается назвать это преимуществом;
2. Запись за границу массива – это неопределённое поведение. Так что дальше уже можно и не рассуждать. Просто так нельзя делать, и всё;
3. Нет гарантии, что переменная будет располагаться в памяти сразу после массива.

А вот другой интересный случай. Давным-давно в 2011 году я написал статью про проверку проекта VirtualDub. Но вместо того, чтобы изменить код, где происходит доступ за границу массива, автор отстаивал, что код работает правильно, а значит, лучше оставить всё как есть: The "error" in f_convolute.cpp.

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

The "error" in f_convolute.cpp

Okay, Mr. Karpov decided to use VirtualDub again as an example of a detected code defect in his article, and while I respect him and his software, I resent the implication that I don't understand how C/C++ arrays work and that he included this example again without noting that the code actually works. I'd like to clarify this here.

This is the structure and reference in question:

struct ConvoluteFilterData {
    long m[9];
    long bias;
    void *dyna_func;
    uint32 dyna_size;
    uint32 dyna_old_protect;
    bool fClip;
};

long rt0=cfd->m[9], gt0=cfd->m[9], bt0=cfd->m[9];

This code is from the general convolution filter, which is one of the oldest filters in VirtualDub. It computes a new image based on the application of a 3x3 grid of coefficients and a bias value. What this code is doing is initializing the color accumulators for the windowing operation with the bias value. The structure in question here is special in that it has a fixed layout that is referenced by many pieces of code, some written in assembly language and some dynamically generated (JITted) code, and so it is known — and required — that the element after the coefficient array (m) is the bias value. As such, this code works as intended, and if someone were to correct the array index to 8 thinking it was an off-by-one error, it would break the code.

That leaves the question of why I over-indexed the array. It's been so long that I don't remember why I did this. It was likely either a result of rewriting the asm routine back into C/C++ — back from when I used to prototype directly in asm — or from refactoring the structure to replace a 10-long array with a 9-long coefficient array and a named bias field. Indexing the tenth element is likely a violation of the C/C++ standard and there's no reason the code couldn't reference the bias field, which is the correct fix. Problem is, the code works as written: the field is guaranteed to be at the correct address and the most likely source of breakage would be the compiler doing aggressive load/store optimizations on individual structure fields. As it happens, the store and load are very far apart — the struct is initialized in the filter start phase and read much later in the per-frame filter loop — and the Visual C++ compiler that I use does not do anything of the sort here, so the generated code works.

The situation at this point is that we're looking at a common issue with acting on static analysis reports, which is making a change to fix a theoretical bug at the risk of introducing a real bug in the process. Any changes to a code base have risk, as the poor guy who added a comment with a backslash at the end knows. As it turns out, this code usually only executes on the image border, so any failures in the field would have been harder to detect, and I couldn't really justify fixing this on the stable branch. I will admit that I have less of an excuse for not fixing it on the dev branch, but honestly that's the least of the problems with that code.

Anyway, that's the history behind the code in f_convolute.cpp, and if you're working with VirtualDub source code, don't change the 9 to an 8.

В общем, у меня реакция, похожая на изображённую на картинке… Не понимаю, почему просто не взять и не написать код, где значение берётся из переменной bias. Код плох, раз понадобилось такое длинное пояснение. Требуется рефакторинг, а не рассказ истории.

Об этой мини-книге

Автор: Карпов Андрей Николаевич. E-Mail: karpov [@] viva64.com.

Более 15 лет занимается темой статического анализа кода и качества программного обеспечения. Автор большого количества статей, посвящённых написанию качественного кода на языке C++. С 2011 по 2021 год удостаивался награды Microsoft MVP в номинации Developer Technologies. Один из основателей проекта PVS-Studio. Долгое время являлся CTO компании и занимался разработкой С++ ядра анализатора. Основная деятельность на данный момент — управление командами, обучение сотрудников и DevRel активность.

Ссылки на полный текст:

• RU — 60 антипаттернов для С++ программиста
• EN — 60 terrible tips for a C++ developer

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