We document errors in Doxygen

This article focuses on the doxygen documentation system verification. This well-known and widely used project, according to a well-founded statement of developers, which has actually become the standard for documenting software written in C ++, has not yet been verified by PVS-Studio. Doxygen scans the source code of the programs and generates documentation on it. The time has come for us to take a look at its sources and see what PVS-Studio can find.

Introduction

Doxygen is a cross-platform source documentation system that has a wide range of target languages: C ++, C, Objective-C, Python, Java, C #, PHP, IDL, Fortran, VHDL and partially supports D. Doxygen generates source-based documentation with comments. of a special kind and can be configured to extract the structure of the program with undocumented sources. Output formats can be HTML, LATEX, man, rtf, xml. Doxygen is used in KDE, Mozilla, Drupal, Pidgin, AbiWorld, FOX toolkit, Torque Game Engine, Crystal Space projects.
')

Preparation and verification

The most recent doxygen sources can be taken from github.com/doxygen/doxygen . Initially, the repository does not contain project files for Visual Studio, but since developers use cmake, they can be generated. I used the console version of the program and the “cmake -G„ Visual Studio 12 ”command to generate the project file for VS 2013. To start the check, just click Check Solution in the PVS-Studio tab in Visual Studio.

Alert Analysis

Before the immediate analysis of warnings, I would like to pay attention to the writing style of doxygen. Often in the code, for some unknown reason, the programmer tried to put everything in one line or neglected the spaces between variables and operators, which significantly reduced the readability of the code. In some places came across strange formatting. And sometimes there was such . To fit some code in the article, I had to format it. After a short digression, I suggest looking at the most interesting errors found in doxygen.

PVS-Studio warning : 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; .... } } 

The if body contains a double assignment of the same variable. Obviously, this is a typo or error when copying. The else block suggests that the value "MemberListType_proSlots" should be written to "* outListType2". Another similar error can be found here: doxygen.cpp 5742 (see the 'da-> type' variable).

The next warning to 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); .... } 

Pay attention to the assignment operation. Most likely, this is a typo and instead of "=" should be used "+ =". On the issue of style, this function does not contain spaces between operators and values ​​in the source code, which significantly worsens the readability of the code. This increases the chance of an error, as it is hard to see the absence of a "+" among the continuous stream of characters. After adding spaces, the error becomes more noticeable. Another similar bug is hidden here:

V519 The 'nn' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 2166, 2167. vhdldocgen.cpp 2167

Let's move on to the next warning.

PVS-Studio warning : 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,"%","%%")); } .... } 

The copy-paste programming method can not only save time when writing code, but also introduce errors into it. Here the code from the if block was copied for use in the else block, but not changed after insertion. Each time you use copy-paste, you should follow the rule “Copy once, check seven times”.

PVS-Studio warning : 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+=":"; .... } .... } 

Similar to the previous warning. In the if block, regardless of the condition, the same character is added to the result string. This is hardly the expected behavior, because then the condition itself does not make sense. And again, I am inclined to think that if, following the generally accepted style, this block was separated into 4 lines, then it would not just look prettier, but also make the typo more obvious. Interestingly, this construct was copied twice for further use in functions, but the error was never noticed. As a result, two more warnings fall on these lines:

• V523 The 'then' statement is equivalent to the 'else' statement. translator_tw.h 1956
• V523 The 'then' statement is equivalent to the 'else' statement. translator_tw.h 1965

PVS-Studio warning : V530. classdef.cpp 1963

 void ClassDef::writeDocumentationContents(....) { QCString pageType = " "; pageType += compoundTypeString(); toupper(pageType.at(1)); .... } 

Here the programmer misunderstood how the toupper function works. He may have expected the function to change the transmitted character to a capital letter. In fact, the function returns the capital equivalent of the symbol-argument, and does not change it. This is how the toupper function is declared in the “ctype.h” header file.

 int toupper (int __c); 

Already from the function declaration, one can see that the parameter is accepted by value, which means that the transmitted symbol cannot be changed. To avoid such errors, it is enough to carefully read the description of the functions used, the behavior of which you are not sure.

PVS-Studio warning:
V560 A part of the 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()) .... } 

This condition will always be false due to the fact that the logical negation of a nonzero value always gives zero. The subsequent use of "AND" does not matter when one of the arguments is zero. As a result, the condition does not depend on the other parameters. Here, it seems more logical to use the bitwise inversion operator ~.

PVS-Studio warning : 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) { .... } .... } 

I’ll say right away that in the body of the for loop, the variable found is not changed. Because of this, the exit condition of the loop depends only on the result of the mmli.current method. This is fraught with the fact that the cycle will always go from start to finish, regardless of whether the desired value was found or not.

PVS-Studio warning:
V595 'bfd ’ Check lines: 3371, 3384. dot.cpp 3371

 void DotInclDepGraph::buildGraph(....) { .... FileDef *bfd = ii->fileDef; QCString url=""; .... url=bfd->getSourceFileBase(); .... if (bfd) .... } 

V595 is perhaps the most common warning among all projects. It is not always before using the pointer that you wonder whether it can be zero. Sometimes you think about it after the second or third use and put the check. But between it and the first dereference, there can be a very large amount of code, which makes the error very difficult to detect. Similar warnings:

• V595 The 'cd' pointer was used before it was verified against nullptr.
  Check lines: 6123, 6131. ​​doxygen.cpp 6123
• V595 The 'p' pointer was used against it.
  Check lines: 1069, 1070. htmldocvisitor.cpp 1069
• V595 The 'Doxygen :: mainPage' pointer was used before it was verified against nullptr.
  Check lines: 3792, 3798. index.cpp 3792
• V595 The 'firstMd' pointer was used before it was verified against nullptr.
  Check lines: 80, 93. membergroup.cpp 80
• V595 The 'lastCompound' pointer was used before it was verified against nullptr.
  Check lines: 410, 420. vhdljjparser.cpp 410
• V595 The 'len' pointer was used against it.
  Check lines: 11960, 11969. qstring.cpp 11960
• V595 The 'len' pointer was used against it.
  Check lines: 11979, 11988. qstring.cpp 11979
• V595 The 'fd' pointer was used before it was verified against nullptr.
  Check lines: 2077, 2085. doxygen.cpp 2077

PVS-Studio warning:
V595 'lne ’pointer 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(); .... } } 

I do not describe the same type of checks, because they are too boring for an article. However, I want to consider another warning V595. Here, the loop is entered only if the return value of li.current () (which is assigned to the lne pointer) is not NULL. This means that inside the loop, the pointer is guaranteed not to be zero, making subsequent verification unnecessary. I wanted to write out this example, because usually the V595 warning indicates a potential dereference of the null pointer, and here it showed an extra check.

Analyzer Warnings: 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; .... } 

The analyzer paid attention to the strange conversion of bool to the class type. In the QCString class, an overloaded assignment operator is not defined for an argument of type bool, but a constructor is defined with an input parameter of type int, which indicates the size of the string. It is this one that is called to create a temporary object with this assignment. The compiler will find a constructor with the argument int and call it, having first converted bool to int. The local variable can have only 2 values: true or false, which corresponds to 1 and 0. In the first case, the designer will create a string from one element. In the second case, you get an empty string. At the end, an assignment statement will be invoked with an argument of type QCString. A similar, but less obvious cast is carried out in the following places:

• V601 The bool type is implicitly cast to the class type. Inspect the fifth argument. context.cpp 2315
• V601 The bool type is implicitly cast to the class type. Inspect the fifth argument. context.cpp 2675
• V601 The bool type is implicitly cast to the class type. Inspect the fifth argument. context.cpp 4456

PVS-Studio Warning: 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); } 

In this section of the code, an uninitialized pointer can be dereferenced. The real code is poorly designed, so this error went unnoticed. For the article, I formatted the code and the error became immediately noticeable. Two more such errors can be found here:

• V614 Potentially uninitialized pointer 'tmpEntry' used. vhdlparser.cc 4451
• V614 Potentially uninitialized pointer 't' used. vhdlparser.cc 5304

PVS-Studio warning : V668 has been defined as 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) .... } 

It is no longer a news for anyone that in the case of an unsuccessful allocation of memory, new throws an exception, and does not return nullptr. This code is a kind of archaic programming. For modern compilers, these checks are no longer meaningful, they can be removed. 3 more such checks:

• V668 allocated r r allocated allocated allocated allocated allocated allocated allocated allocated allocated allocated allocated allocated The exception will be generated in the case of memory allocation error. template.cpp 1981
• V668 allocated allocated allocated allocated allocated allocated allocated allocated allocated allocated allocated allocated allocated The exception will be generated in the case of memory allocation error. qglist.cpp 1005
• V668 has been defined as the 'allocated allocated allocated allocated allocated allocated allocated allocated. The exception will be generated in the case of memory allocation error. qstring.cpp 12099

PVS-Studio warning : 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; .... } 

The analyzer detected an incorrect use of the “realloc” function. In case of unsuccessful allocation of memory, “realloc” will return nullptr, overwriting the previous value of the pointer. To prevent this, it is recommended to store the value of the pointer in a temporary variable before using realloc. In total, in addition to this, 8 potential memory leaks were found in the project:

• V701 realloc () possible leak: when realloc () fails in allocating memory, the original pointer 'd' is lost. Consider assigning realloc () to a temporary pointer. qcstring.h 396
• V701 realloc () possible leak: when realloc () fails in allocating memory, the original pointer 'str' is lost. Consider assigning realloc () to a temporary pointer. growbuf.h 16
• V701 realloc () possible leak: when realloc () fails in allocating memory, the original pointer 'str' is lost. Consider assigning realloc () to a temporary pointer. growbuf.h 23
• V701 realloc () possible leak: when realloc () fails in allocating memory, the original pointer 'str' is lost. Consider assigning realloc () to a temporary pointer. growbuf.h 33
• V701 realloc () possible leak: when realloc () fails in allocating memory, the original pointer 'm_str' is lost. Consider assigning realloc () to a temporary pointer. vhdlstring.h 61
• V701 realloc () possible leak: when realloc () fails in allocating memory, the original pointer 'shd-> data' is lost. Consider assigning realloc () to a temporary pointer. qgarray.cpp 224
• V701 realloc () possible leak: when realloc () fails in allocating memory, the original pointer 'm_data' is lost. Consider assigning realloc () to a temporary pointer. qgstring.cpp 114
• V701 realloc () possible leak: when realloc () fails in allocating memory, the original pointer 'm_data' is lost. Consider assigning realloc () to a temporary pointer. qgstring.cpp 145

Conclusion

In conclusion, I would like to say that the analyzer coped well with its work and found many suspicious places, despite the fact that doxygen is a popular project and is widely used by a large number of both small and large companies. I gave the most basic warnings and left such uninteresting as extra checks, unused variables, etc. beyond the scope of this article. It is worth mentioning that in some places I was surprised by the very careless, in my opinion, code design.

I would like to wish the readers to write beautiful, readable code and avoid mistakes. And if the first entirely depends on the programmer, then the analyzer can help with the second. Download and try free PVS-Studio on your project here: http://www.viva64.com/ru/pvs-studio-download/


If you want to share this article with an English-speaking audience, then please use the link to the translation: Igor Shtukarev. Documenting Bugs in Doxygen .