В этой статье речь пойдет о проверке системы документирования doxygen. Этот известный и широко используемый проект, по имеющему основания заявлению разработчиков, ставший фактически стандартом для документирования программного обеспечения, написанного на языке C++, еще не был проверен PVS-Studio. Doxygen просматривает исходный код программ и генерирует по нему документацию. Пришло время нам заглянуть в его исходники и посмотреть, что сможет найти PVS-Studio.
Doxygen - кроссплатформенная система документирования исходных текстов, которая имеет широкий набор целевых языков: C++, C, Objective-C, Python, Java, C#, PHP, IDL, Fortran, VHDL и частично поддерживает D. Doxygen генерирует документацию на основе исходных текстов с комментариями особого вида и может быть настроен на извлечение структуры программы с недокументированными исходниками. Выходными форматами могут быть HTML, LATEX, man, rtf, xml. Doxygen используется в проектах KDE, Mozilla, Drupal, Pidgin, AbiWorld, FOX toolkit, Torque Game Engine, Crystal Space.
Самые свежие исходники doxygen могут быть взяты с github.com/doxygen/doxygen. Изначально репозиторий не содержит проектных файлов для Visual Studio, но так как разработчики используют cmake, то их можно сгенерировать. Я воспользовался консольной версией программы и командой "cmake -G "Visual Studio 12"" для генерации проектного файла для VS 2013. Для запуска проверки достаточно нажать Check Solution во вкладке PVS-Studio в Visual Studio.
Перед непосредственным разбором предупреждений хочется обратить внимание на стиль написания doxygen. Зачастую в коде программист по непонятным причинам стремился уложить все в одну строку или пренебрегал пробелами между переменными и операторами, что существенно снижало читаемость кода. В некоторых местах попадалось странное форматирование. Чтобы уместить некоторый код в статье, мне пришлось отформатировать его. После небольшого отступления предлагаю посмотреть на самые интересные ошибки, найденные в doxygen.
Предупреждение PVS-Studio: V519 The '* outListType1' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 8326, 8327. util.cpp 8327
void convertProtectionLevel(MemberListType inListType,
int *outListType1,
int *outListType2)
{
static bool extractPrivate;
....
switch (inListType)
{
....
case MemberListType_priSlots:
if (extractPrivate)
{
*outListType1=MemberListType_pubSlots;
*outListType1=MemberListType_proSlots; <<<<====
}
else
{
*outListType1=-1;
*outListType2=-1;
}
break;
....
}
}В теле if содержится двойное присваивание одной и той же переменной. Очевидно, что это опечатка или ошибка при копировании. Блок else подсказывает, что значение "MemberListType_proSlots" должно быть записано в "*outListType2". Ещё одну подобную ошибку можно найти здесь: doxygen.cpp 5742 (см. переменную 'da->type').
Следующее предупреждение PVS-Studio: V519 The 'pageTitle' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 970, 971. vhdldocgen.cpp 971
QCString VhdlDocGen::getClassTitle(const ClassDef *cd)
{
QCString pageTitle;
if (cd == 0)
return "";
pageTitle += cd->displayName();
pageTitle = VhdlDocGen::getClassName(cd);
....
}Обратите внимание на операцию присваивания. Скорее всего, это опечатка и вместо "=" должно использоваться "+=". К вопросу о стиле, в исходном коде эта функция не содержит пробелов между операторами и значениями, что существенно ухудшает читаемость кода. Это увеличивает шанс возникновения ошибки, так как тяжело увидеть отсутствие "+" среди непрерывного потока символов. После добавления пробелов ошибка становится более заметной. Еще одна похожая ошибка прячется тут:
V519 The 'nn' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2166, 2167. vhdldocgen.cpp 2167
Перейдем к следующему предупреждению.
Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. docparser.cpp 521
static void checkUndocumentedParams()
{
....
if (g_memberDef->inheritsDocsFrom())
{
warn_doc_error(g_memberDef->getDefFileName(),
g_memberDef->getDefLine(),
substitute(errMsg,"%","%%"));
}
else
{
warn_doc_error(g_memberDef->getDefFileName(),
g_memberDef->getDefLine(),
substitute(errMsg,"%","%%"));
}
....
}Метод программирования copy-paste может не только сэкономить время при написании кода, но и привнести в него ошибки. Здесь код из блока if был скопирован для использования в блоке else, но не изменен после вставки. При каждом использовании copy-paste следует придерживаться правила "Один раз скопируй, семь раз проверь".
Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. translator_tw.h 769
class TranslatorChinesetraditional : public Translator
{
public:
....
virtual QCString trGeneratedFromFiles(bool single, ....)
{
....
QCString result=(QCString)"?";
....
if (single) result+=":"; else result+=":";
....
}
....
}Аналогичное предыдущему предупреждение. В блоке if в независимости от условия к строке result прибавляется один и тот же символ. Вряд ли это ожидаемое поведение, ведь тогда само условие не имеет смысла. И снова я склоняюсь к тому, что если бы, следуя общепринятому стилю, этот блок был разнесен на 4 строки, то он не просто бы выглядел красивее, но и сделал бы опечатку более очевидной. Интересно, что эта конструкция была дважды скопирована для дальнейшего использования в функциях, но ошибка так и не была замечена. В итоге еще два предупреждения приходятся на эти строчки:
Предупреждение PVS-Studio: V530 The return value of function 'toupper' is required to be utilized. classdef.cpp 1963
void ClassDef::writeDocumentationContents(....)
{
QCString pageType = " ";
pageType += compoundTypeString();
toupper(pageType.at(1));
....
}Тут программистом был неверно понят принцип работы функции toupper. Возможно, он ожидал, что функция изменит переданный символ на заглавную букву. На самом деле функция возвращает заглавный эквивалент символа-аргумента, а не изменяет его. Вот как объявлена функция toupper в заголовочном файле "ctype.h".
int toupper (int __c);Уже из объявления функции видно, что параметр принимается по значению, а значит переданный символ не может быть изменен. Чтобы не допускать таких ошибок, достаточно внимательно читать описание к используемым функциям, в поведении которых вы не уверены.
Предупреждение PVS-Studio: V560 A part of conditional expression is always false: (flags() &!0x0008). qfile_win32.cpp 267
#define IO_Truncate 0x0008
bool QFile::open(....)
{
....
int length = INT_MAX;
if ((flags() & !IO_Truncate) && length == 0 && isReadable())
....
}Данное условие всегда будет ложным из-за того, что логическое отрицание ненулевого значения всегда дает ноль. Последующее применение "И" не имеет значения, когда один из аргументов является нулем. В результате условие не зависит от остальных параметров. Более логичным здесь кажется использование оператора побитовой инверсии '~'.
Предупреждение PVS-Studio: V560 A part of conditional expression is always true: !found. util.cpp 4264
bool getDefs(....)
{
....
bool found=FALSE;
MemberListIterator mmli(*mn);
MemberDef *mmd;
for (mmli.toFirst();((mmd=mmli.current()) && !found);++mmli)
{
....
}
....
}Сразу скажу, что в теле цикла for переменная found не изменяется. Из-за этого условие выхода из цикла зависит только от результата метода mmli.current. Это чревато тем, что цикл всегда будет проходить от начала до конца в независимости от того, было ли найдено искомое значение или нет.
Предупреждение PVS-Studio: V595 The 'bfd' pointer was utilized before it was verified against nullptr. Check lines: 3371, 3384. dot.cpp 3371
void DotInclDepGraph::buildGraph(....)
{
....
FileDef *bfd = ii->fileDef;
QCString url="";
....
url=bfd->getSourceFileBase();
....
if (bfd)
....
}V595 является, пожалуй, самым распространенным предупреждением среди всех проектов. Не всегда перед использованием указателя задумываешься, может ли он быть нулевым. Порой об этом вспоминаешь после второго или третьего использования и ставишь проверку. Но между ней и первым разыменованием может быть очень большое количество кода, что делает ошибку весьма трудной для обнаружения. Аналогичные предупреждения:
Предупреждение PVS-Studio: V595 The 'lne' pointer was utilized before it was verified against nullptr. Check lines: 4078, 4089. index.cpp 4078
static void writeIndexHierarchyEntries(OutputList &ol, ....)
{
QListIterator<LayoutNavEntry> li(entries);
LayoutNavEntry *lne;
for (li.toFirst();(lne=li.current());++li)
{
LayoutNavEntry::Kind kind = lne->kind();
....
bool addToIndex=lne==0 || lne->visible();
....
}
}Я не описываю однотипные проверки, потому что они слишком скучны для статьи. Тем не менее я хочу рассмотреть ещё одно предупреждение V595. Здесь вход в цикл осуществляется, только если возвращаемое значение li.current() (которое присваивается указателю lne) будет не NULL. Это значит, что внутри цикла указатель гарантированно не будет нулевым, делая последующую проверку ненужной. Мне захотелось выписать этот пример, потому что обычно предупреждение V595 указывает на потенциальное разыменование нулевого указателя, а здесь оно показало лишнюю проверку.
Предупреждения анализатора: V601 The bool type is implicitly cast to the class type. docsets.cpp 473
struct IncludeInfo
{
....
bool local;
};
void DocSets::addIndexItem(Definition *context,MemberDef *md,
const char *,const char *)
{
QCString decl;
....
IncludeInfo *ii = cd->includeInfo();
....
decl=ii->local;
....
}Анализатор обратил внимание на странное приведение bool к типу класса. В классе QCString не определен перегруженный оператор присваивания для аргумента типа bool, но определен конструктор с входным параметром типа int, который обозначает размер строки. Именно он вызывается для создания временного объекта при данном присваивании. Компилятор найдет конструктор с аргументом int и вызовет его, предварительно преобразовав bool к int. Переменная local может иметь только 2 значения: true или false, что соответствует 1 и 0. В первом случае конструктором будет создана строка из одного элемента. Во втором случае получится пустая строка. В конце будет вызван оператор присваивания с аргументом типа QCString. Похожее, но менее очевидное приведение осуществляется в следующих местах:
Предупреждение PVS-Studio: V614 Potentially uninitialized pointer 't' used. vhdlparser.cc 4127
QCString VhdlParser::extended_identifier()
{
Token *t;
if (!hasError)
t = jj_consume_token(EXTENDED_CHARACTER);
return t->image.c_str();
assert(false);
}В этом участке кода возможно разыменование неинициализированного указателя. Реальный код плохо оформлен, поэтому эта ошибка и осталась незамеченной. Для статьи я отформатировал код и ошибка стала сразу хорошо заметна. Еще две таких ошибки можно найти здесь:
Предупреждение PVS-Studio: V668 There is no sense in testing the 'file' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. outputgen.cpp 47
void OutputGenerator::startPlainFile(const char *name)
{
....
file = new QFile(fileName);
if (!file)
....
}Ни для кого уже не новость, что new в случае неудачного выделения памяти выбрасывает исключение, а не возвращает nullptr. Этот код - своеобразный архаизм программирования. Для современных компиляторов эти проверки больше не имеют смысла, их можно удалить. Еще 3 таких проверки:
Предупреждение PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'd' is lost. Consider assigning realloc() to a temporary pointer. qcstring.h 396
class BufStr
{
public:
....
void resize(uint newlen)
{
....
m_buf = (char *)realloc(m_buf,m_size);
....
}
private:
uint m_size;
char *m_buf;
....
}Анализатор обнаружил неправильное использование функции "realloc". При неудачном выделении памяти "realloc" вернет nullptr, перезаписав предыдущее значение указателя. Чтобы предотвратить это, рекомендуется сохранять значение указателя во временной переменной перед использованием "realloc". Всего, помимо этой, в проекте было найдено 8 потенциальных утечек памяти:
В заключении хочется сказать, что анализатор прекрасно справился со своей работой и нашел много подозрительных мест, несмотря на то, что doxygen является популярным проектом и широко используется большим количеством как мелких, так и крупными компаниями. Я привел самые основные предупреждения и оставил такие неинтересные, как лишние проверки, неиспользуемые переменные и т.п. за рамками данной статьи. Стоит упомянуть, что в некоторых местах меня удивило весьма небрежное, по моему мнению, оформление кода.
Хочется пожелать читателям писать красивый, читаемый код и не допускать ошибок. И если первое целиком зависит от программиста, то со вторым может помочь анализатор. Скачать и бесплатно попробовать PVS-Studio на своем проекте можно здесь: http://www.viva64.com/ru/pvs-studio/download/