Это третья статья, где я хочу рассказать про новую пару приёмов при программировании, которые помогут сделать код более простым и надежным.
С предыдущими двумя заметками можно познакомиться здесь [1] и здесь [2]. В этот раз примеры будут взяты из проекта Qt.
Проект Qt 4.7.3. попался мне для изучения не случайно. Пользователи PVS-Studio обратили внимание, что анализ как-то слабоват, если дело касается проверки проектов, построенных на основе библиотеки Qt. Это не удивительно. Статический анализ позволяет находить ошибки за счет того, что смотрит на код более высокоуровнево, чем компилятор. Следовательно, он должен знать определенные паттерны кода и, что делают функции различных библиотек. В противном случае, он пройдет мимо многих замечательных ляпов. Поясню на примере:
if (strcmp(My_Str_A, My_Str_A) == 0)Бессмысленно сравнивать строку саму с собой. Но компилятор промолчит. Он не задумывается, в чем суть функции strcmp(). У него хватает своих забот. А вот статические анализаторы могут заподозрить неладное. В Qt есть своя разновидность функции сравнения строк - qstrcmp(). И, соответственно, анализатор нужно обучить обращать внимание и на такую строку:
if (qstrcmp(My_Str_A, My_Str_A) == 0)Осваивание библиотеки Qt и создание специализированных проверок - это большая и планомерная работа. И началом этой работы стала проверка самой библиотеки.
Закончив просмотр предупреждений, у меня созрело несколько новых мыслей по улучшению кода, которые, как я надеюсь, будут вам интересны и полезны.
Код библиотеки Qt очень качественен и практически не содержит ошибок. Зато в нём обнаружилось большое количество излишних инициализаций, излишних сравнений или лишних копирования значений переменных.
Приведу пару примеров для ясности:
QWidget *WidgetFactory::createWidget(...)
{
...
} else if (widgetName == m_strings.m_qDockWidget) { <<<===
w = new QDesignerDockWidget(parentWidget);
} else if (widgetName == m_strings.m_qMenuBar) {
w = new QDesignerMenuBar(parentWidget);
} else if (widgetName == m_strings.m_qMenu) {
w = new QDesignerMenu(parentWidget);
} else if (widgetName == m_strings.m_spacer) {
w = new Spacer(parentWidget);
} else if (widgetName == m_strings.m_qDockWidget) { <<<===
w = new QDesignerDockWidget(parentWidget);
...
}Здесь два раза продублировано одно и то же сравнение. Это не ошибка, но совершенно избыточный код. Другой аналогичный пример:
void QXmlStreamReaderPrivate::init()
{
tos = 0; <<<===
scanDtd = false;
token = -1;
token_char = 0;
isEmptyElement = false;
isWhitespace = true;
isCDATA = false;
standalone = false;
tos = 0; <<<===
...
}Вновь не ошибка, но совершенно ненужная двойная инициализация переменной. И подобных продублированных действий я насчитал очень много. Возникают они из-за длинных списков сравнений, присваиваний, инициализаций. Просто не видно, что переменная уже обрабатывается, из-за чего и появляются лишние операции. Я могу назвать три неприятных последствия наличия таких дублирующихся действий:
1. Дубликаты увеличивают длину кода. А чем длиннее код, тем легче добавить еще один дубликат.
2. Если мы захотим изменить логику программы и удалим одну проверку или одно присваивание, то дубликат этого действия может подарить вам несколько часов увлекательной отладки. Сами представьте, вы пишите "tos = 1" (см. первый пример), а потом в другой части программы удивляетесь, отчего же по-прежнему "tos" равен нулю.
3. Замедление скорости работы. Практически всегда им можно пренебречь в таких ситуациях, но оно всё-таки есть.
Надеюсь, убедил, что дубликатам не место в нашем коде. Как с ними бороться? Как правило, подобные инициализации/сравнения идут блоком. И есть такой же блок переменных. Рационально писать код так, чтобы порядок объявлений переменных и порядок работы с ними совпадал. Приведу пример не очень хорошего кода:
struct T {
int x, y, z;
float m;
int q, w, e, r, t;
} A;
...
A.m = 0.0;
A.q = 0;
A.x = 0;
A.y = 0;
A.z = 0;
A.q = 0;
A.w = 0;
A.r = 1;
A.e = 1;
A.t = 1;Естественно, это схематичный пример. Смысл в том, что когда инициализация не последовательна, то намного легче написать две одинаковых строки. В приведенном коде дважды инициализируется переменная 'q'. И ошибка плохо заметна, если просматривать код бегло. Если теперь инициализировать переменные в той же последовательности, как они объявлены, то подобной ошибке просто не будет места. Улучшенный вариант кода:
struct T {
int x, y, z;
float m;
int q, w, e, r, t;
} A;
...
A.x = 0;
A.y = 0;
A.z = 0;
A.m = 0.0;
A.q = 0;
A.w = 0;
A.e = 1;
A.r = 1;
A.t = 1;Конечно, я знаю, что не всегда так можно написать и работать с переменными в порядке их объявления. Но часто это возможно и полезно. Дополнительным плюсом станет то, что вам легче будет ориентироваться в коде.
Рекомендация. Добавляя новую переменную, постарайтесь, чтобы её инициализация и обработка, осуществлялась в соответствии с её положением относительно других переменных.
Про табличные методы хорошо написано у С. Макконнелла в книге "Совершенный код" в главе N18 [3]:
Табличный метод — это схема, позволяющая искать информацию в таблице, а не использовать для этого логические выражения, такие как if и case. Практически все, что вы можете выбирать посредством логических операторов, можно выбирать, применяя таблицы. В простых случаях логические выражения проще и понятней. Но при усложнении логических построений таблицы становятся всё привлекательнее.
Так вот, очень жаль, что программисты по-прежнему любят огромные switch() или густой лес конструкций if-else. Перебороть в себе это очень сложно. Кажется, ну еще-то один "case:" или маленький "if" не повредит. Но он вредит. И неудачно добавляют новые условия даже самые опытные программисты. В качестве примера пара дефектов, найденных в Qt.
int QCleanlooksStyle::pixelMetric(...)
{
int ret = -1;
switch (metric) {
...
case PM_SpinBoxFrameWidth:
ret = 3;
break;
case PM_MenuBarItemSpacing:
ret = 6;
case PM_MenuBarHMargin:
ret = 0;
break;
...
}Длинный-предлинный switch(). И, естественно, имеется забытый оператор "break". Анализатор выявил эту ошибку за счет того, что переменной "ret" дважды подряд присваивается разное значение.
Пожалуй, намного лучше, было бы завести какой-то std::map<PixelMetric, int> и явно табличкой задать соответствие между метрикой и числами. Можно придумать и другие табличные варианты реализации функции.
Еще один пример:
QStringList ProFileEvaluator::Private::values(...)
{
...
else if (ver == QSysInfo::WV_NT)
ret = QLatin1String("WinNT");
else if (ver == QSysInfo::WV_2000)
ret = QLatin1String("Win2000");
else if (ver == QSysInfo::WV_2000) <<<=== 2003
ret = QLatin1String("Win2003");
else if (ver == QSysInfo::WV_XP)
ret = QLatin1String("WinXP");
...
}В коде два раза сравниваем переменную 'ver' с константой WV_2000. Хороший пример, где самое место табличному методу. Например, такой метод мог бы выглядеть так:
struct {
QSysInfo::WinVersion m_ver;
const char *m_str;
} Table_WinVersionToString[] = {
{ WV_Me, "WinMe" },
{ WV_95, "Win95" },
{ WV_98, "Win98" },
{ WV_NT, "WinNT" },
{ WV_2000, "Win2000" },
{ WV_2003, "Win2003" },
{ WV_XP, "WinXP" },
{ WV_VISTA,"WinVista" }
};
ret = QLatin1String("Unknown");
for (size_t i = 0; i != count_of(Table_WinVersionToString); ++i)
if (Table_WinVersionToString[i].m_ver == ver)
ret = QLatin1String(Table_WinVersionToString[i].m_str);Конечно, это просто прототип, но он хорошо демонстрирует идею табличных методов. Согласитесь, что выявить в такой таблице ошибку намного проще.
Рекомендация. Не ленитесь написать функцию с использованием табличных методов. Да, это займет немного времени, но зато это окупится потом. Вам будет проще и быстрее добавлять новые условия, а вероятность ошибки при этом будет намного ниже.
Поскольку Qt большая библиотека, то, несмотря на высокое качество, в ней можно встретить разнообразнейшие ошибки. Действует закон больших чисел. Размер *.cpp, *.h и аналогичных файлов проекта Qt составляет около 250 мегабайт. Как не маловероятна ошибка, в большом коде её встретить вполне реально. На основании других ошибок, которые я обнаружил в Qt, составить какие-то рекомендации сложно. Просто опишу некоторые ошибки, которые мне понравились.
QString decodeMSG(const MSG& msg)
{
...
int repCount = (lKeyData & 0xffff); // Bit 0-15
int scanCode = (lKeyData & 0xf0000) >> 16; // Bit 16-23
bool contextCode = (lKeyData && 0x20000000); // Bit 29
bool prevState = (lKeyData && 0x40000000); // Bit 30
bool transState = (lKeyData && 0x80000000); // Bit 31
...
}Случайно используется оператор && вместо &. Обратите внимание, как полезно иметь в коде комментарии. Сразу становится понятно, что это действительно ошибка и как на самом деле должны обрабатываться биты.
Следующий пример будет на тему длинных выражений:
static ShiftResult shift(...)
{
...
qreal l = (orig->x1 - orig->x2)*(orig->x1 - orig->x2) +
(orig->y1 - orig->y2)*(orig->y1 - orig->y1) *
(orig->x3 - orig->x4)*(orig->x3 - orig->x4) +
(orig->y3 - orig->y4)*(orig->y3 - orig->y4);
...
}Видите ошибку? Вот-вот, с ходу и не заметишь. Хорошо, подскажу. Беда вот здесь: "orig->y1 - orig->y1". Ещё меня третье умножение смущает, но возможно так и надо.
Да, еще вопрос. А у вас ведь тоже в программах вот такие вот блоки вычислений есть? Не пора ли попробовать статический анализатор кода PVS-Studio? Так, порекламировал. Пойдем дальше.
Использование неинициализированных переменных. Их можно найти в любом большом приложении:
PassRefPtr<Structure>
Structure::getterSetterTransition(Structure* structure)
{
...
RefPtr<Structure> transition = create(
structure->storedPrototype(), structure->typeInfo());
transition->m_propertyStorageCapacity =
structure->m_propertyStorageCapacity;
transition->m_hasGetterSetterProperties =
transition->m_hasGetterSetterProperties;
transition->m_hasNonEnumerableProperties =
structure->m_hasNonEnumerableProperties;
transition->m_specificFunctionThrashCount =
structure->m_specificFunctionThrashCount;
...
}Здесь опять нужно дать подсказу, чтобы долго не мучить ваши глаза. Смотреть надо на инициализацию переменной "transition->m_hasGetterSetterProperties".
Уверен, почти каждый из нас, когда только начинал программировать, делал ошибку в духе:
const char *p = ...;
if (p == "12345")И только потом приходило осознание, зачем нужны такие на первый взгляд странные функции, как strcmp(). К сожалению, язык Си++ настолько суров, что сделать такую ошибку можно и через много лет, будучи профессиональным разработчиком с опытом:
const TCHAR* getQueryName() const;
...
Query* MultiFieldQueryParser::parse(...)
{
...
if (q && (q->getQueryName() != _T("BooleanQuery") ...
...
}Что бы еще показать. Вот, например, неправильно написанный обмен значений переменных.
bool qt_testCollision(...)
{
...
t=x1; x1=x2; x2=t;
t=y1; x1=y2; y2=t;
...
}Это пример того, что можно ошибиться даже в очень простом коде. Так, пока ещё не было примеров на тему выхода за границы массива. Сейчас будет:
bool equals( class1* val1, class2* val2 ) const
{
...
size_t size = val1->size();
...
while ( --size >= 0 ){
if ( !comp(*itr1,*itr2) )
return false;
itr1++;
itr2++;
}
...
}Условие "‑‑size >= 0" всегда истинно, так как переменная size имеет беззнаковый тип. Если будут сравниваться одинаковые последовательности, то произойдет выход за границы массивов.
Можно продолжать и дальше. Надеюсь, вы как программисты понимаете, что ошибки в проекте такого объема описывать в одной статье нет никакой возможности. Поэтому последнее, на закуску:
STDMETHODIMP QEnumPins::QueryInterface(const IID &iid,void **out)
{
...
if (S_OK)
AddRef();
return hr;
}Должно было быть что-то в духе "if (hr == S_OK)" или "if (SUCCEEDED(hr))". Макрос S_OK есть не что иное, как 0. Поэтому бяка с неправильным подсчетом количества ссылок здесь неизбежна.
Спасибо за внимание. Используйте статический анализ кода, и вы сможете сэкономить массу времени для более полезных дел, чем отладка и сопровождение кода.
Также я буду благодарен читателям, если вы пришлете мне интересные примеры ошибок, которые вы находили в своём или чужом коде и, для которых можно реализовать диагностические правила.