# Let's check the qdEngine game engine, part one: top 10 warnings issued by PVS\-Studio The bugs found in the qdEngine game engine are quite diverse, so I don't want to put them all in one publication\. Our readers may miss out on interesting topics on writing high\-quality code\. That's why the project analysis will be a series of articles, and the first one is dedicated to the most interesting warnings issued by the PVS\-Studio plugin\. ![1119_qdEngine_check/image1.png](https://import.viva64.com/docx/blog/1119_qdEngine_check/image1.png) ## Best button K\-D Lab has released the source code of qdEngine, a game engine for creating quests\. The engine is mostly written in C\+\+ and looks like a good candidate for checking with the PVS\-Studio code analyzer\. The analyzer has found various types of errors in the project\. So, we'd like to look at them from different angles\. That's why I'll publish several articles on different topics\. The first one is dedicated to the **Best** button in the [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) plugins\. ![1119_qdEngine_check/image2.png](https://import.viva64.com/docx/blog/1119_qdEngine_check/image2.png) The button is a great help for a user to get started with the PVS\-Studio tool for the first time\. The analyzer selects 10 warnings that are most likely to indicate real errors\. They should also be diverse and interesting\. All static code analyzers issue a lot of warnings on the first run, many of these warnings are likely to be irrelevant or false\. It's not a big deal because analyzers, including PVS\-Studio, have flexible settings\. However, programmers new to the static analysis methodology may be discouraged by a bunch of warnings that seem irrelevant to them\. They'd like to see some interesting bugs right away\. The **Best** button makes that possible\. When you click it, the analyzer selects 10 warnings using the empirical algorithm\. When making a choice, the analyzer isn't only guided by the probability that it's a real error, it also tries to select warnings of different types\.
About error certainty\\\. Let me explain why picking 10 warnings that are definitely real errors isn't the best option\. Let's look at the [V668](https://pvs-studio.com/en/docs/warnings/v668/) diagnostic rule as an example\. It indicates a pointless check of the pointer returned by the operator _new_\. Here's the real code fragment from the Minetest project: ```cpp clouds = new Clouds(smgr, -1, time(0)); if (!clouds) { *error_message = "Memory allocation error (clouds)"; errorstream << *error_message << std::endl; return false; } ``` If the analyzer has detected such code, we're dealing with a real error in code\. Of course, there's still a _nothrow_ version of the operator _new_, but the analyzer considers it, too\. There are many such errors\. In the whole time we've been writing articles, we've already found 1630 similar bugs in open\-source projects\! It's very likely that the analyzer would find similar fragments in a project that you'd use to get started with it\. However, issuing too many such warnings is undesirable: 1. It's not that interesting\. You wouldn't like to see seven identical warnings about _new_, for example\. 1. Certainty doesn't mean criticality\. That's exactly the case\. Here's an unreachable code fragment\. It's not good, but I'm almost sure that the application works as intended\. More complex diagnostic rules usually detect more interesting and critical errors\. The more complex the error we're looking for, the harder it's to guarantee that we found a real bug\. Even a human has to puzzle over such code\.
For the qdEngine project, the button has worked well: * eight warnings indicate bugs or bad code; * one warning is technically correct, but there is no real error; * one warning is a false positive\. I think all these warnings show the analyzer capabilities really well\. We're going to look at them\. I hope this will encourage people who have only read about PVS\-Studio to give it a try\. [Download](https://pvs-studio.com/en/pvs-studio/download/) the analyzer, install it, check the project, and click **Best**\. If something goes wrong, just contact our support team\. We're always ready to help you\. ## Warnings Let's have a look at the promised warnings and practice some refactoring a bit\. ### Warning N1: duplicate ```cpp bool qdGameObject::init() { .... drop_flag(QD_OBJ_STATE_CHANGE_FLAG | QD_OBJ_IS_IN_TRIGGER_FLAG | QD_OBJ_STATE_CHANGE_FLAG | QD_OBJ_IS_IN_INVENTORY_FLAG); .... } ``` The PVS\-Studio warning: [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions 'QD\_OBJ\_STATE\_CHANGE\_FLAG' to the left and to the right of the '\|' operator\. qd\_game\_object\.cpp 176 The _QD\_OBJ\_STATE\_CHANGE\_FLAG_ constant, declared in _qd\_game\_object\.h_ next to many others, is used twice: ```cpp .... const int QD_OBJ_HAS_BOUND_FLAG = 0x80; const int QD_OBJ_DISABLE_MOVEMENT_FLAG = 0x100; const int QD_OBJ_DISABLE_MOUSE_FLAG = 0x200; const int QD_OBJ_IS_IN_TRIGGER_FLAG = 0x400; const int QD_OBJ_STATE_CHANGE_FLAG = 0x800; const int QD_OBJ_IS_IN_INVENTORY_FLAG = 0x1000; const int QD_OBJ_KEYBOARD_CONTROL_FLAG = 0x2000; .... ``` Developers probably forgot to use some other constant to create a mask\. Perhaps it's just a duplicate, and we can remove the repeating constant from the expression\. It's hard to say for sure, as I'm not familiar with the project code\. ### Warning N2: typo \(lost \# character\) A very beautiful error, in my opinion\. I admit I may have weird tastes and professional deformations, but I hope you can appreciate this bug, too\. ```cpp const qdGameObjectStateWalk* qdGameObjectMoving::current_walk_state() const { const qdGameObjectState* st = get_cur_state(); if(!st || st -> state_type() != qdGameObjectState::STATE_WALK){ #ifndef _QUEST_EDITOR st = last_walk_state_; if(!st || st -> state_type() != qdGameObjectState::STATE_WALK) st = get_default_state(); else st = get_default_state(); if(!st) st = get_state(0); #endif } .... } ``` The PVS\-Studio warning: [V523](https://pvs-studio.com/en/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. qd\_game\_object\_moving\.cpp 2781 The analyzer points out that the same action is performed regardless of the condition: ```cpp if(!st || st -> state_type() != qdGameObjectState::STATE_WALK) st = get_default_state(); else st = get_default_state(); ``` Note that the code is formatted in a strange way\. The _else_ keyword is at the beginning of the string\. If we look at the code next to it and think about it a bit, we can see that the developers really wanted to write the _\#else_ preprocessor directive but not the _else_ statement\. However, the programmer made a typo and forgot to include the hash symbol \(_\#_\)\. Here's the fixed code: ```cpp const qdGameObjectState* st = get_cur_state(); if(!st || st -> state_type() != qdGameObjectState::STATE_WALK){ #ifndef _QUEST_EDITOR st = last_walk_state_; if(!st || st -> state_type() != qdGameObjectState::STATE_WALK) st = get_default_state(); #else st = get_default_state(); if(!st) st = get_state(0); #endif ``` ### Warning N3: unused string ```cpp bool qdGameScene::adjust_files_paths(....) { .... for(qdGameObjectList::const_iterator it = object_list().begin(); it != object_list().end(); ++it) { if((*it) -> named_object_type() == QD_NAMED_OBJECT_STATIC_OBJ) { qdGameObjectStatic* obj = static_cast(*it); if(obj -> get_sprite() -> file()) QD_ADJUST_TO_REL_FILE_MEMBER(pack_corr_dir, obj -> get_sprite() -> file, obj -> get_sprite() -> set_file, can_overwrite, all_ok); std::string str = obj -> get_sprite() -> file(); str = str; } } .... } ``` The PVS\-Studio warning: [V570](https://pvs-studio.com/en/docs/warnings/v570/) The 'str' variable is assigned to itself\. qd\_game\_scene\.cpp 1799 In the loop, the file list is operated in some way\. As I understand it, the developers added this code fragment for debugging purposes, and the analyzer doesn't like it: ```cpp std::string str = obj -> get_sprite() -> file(); str = str; ``` In the first line, the file name is taken\. The second line, in my opinion, serves two purposes: 1. It's convenient to set a breakpoint on it in the debugger and immediately see the file name in the _str_ variable\. 1. Assigning an object to itself may also remove some compiler/analyzer warnings that the _str_ variable is unused\. There's no error here\. However, this is a code smell, and it's worth eliminating\. In other words, the analyzer warning is a good reason to do some refactoring\. I'd start by changing the interface of the _file_ function in the _qdSprite_ class: ```cpp const char* file() const { return file_.c_str(); } ``` It's strange to return the contents of _std::string_ as just a pointer and then check its parts for _nullptr_\. In reality, the function never returns _nullptr_\. If we need a pointer somewhere in the code, we can always call _c\_str_ there\. The changed function looks like this: ```cpp const std::string& file() const { return file_; } ``` Of course, a change to the function interface affects a large amount of code in the project\. For symmetry, it also makes sense to change the _set\_file_ function\. I have a feeling that all this would be good for the code\. We're moving on\. Let's rewrite the code fragment we've discussed earlier\. ```cpp if((*it) -> named_object_type() == QD_NAMED_OBJECT_STATIC_OBJ) { qdGameObjectStatic* obj = static_cast(*it); const std::string &file = obj -> get_sprite() -> file(); QD_ADJUST_TO_REL_FILE_MEMBER(pack_corr_dir, file, obj -> get_sprite() -> set_file, can_overwrite, all_ok); } ``` We've made the following enhancements: 1. The code is now shorter and simpler\. 1. The pointless pointer check is gone\. 1. There's no creation of a temporary _std::string_ object in the loop just to make debugging easier\. These are relatively resource\-intensive operations due to memory allocation and deallocation\. Although, there's no guarantee that the compiler will remove all the unnecessary stuff during optimization\. 1. The PVS\-Studio analyzer no longer issues the [V570](https://pvs-studio.com/en/docs/warnings/v570/) warning\. 1. For debugging purposes, we can also set a breakpoint \(on the line with the _QD\_ADJUST\_TO\_REL\_FILE\_MEMBER_ macro\) and see the file name\. ### Warning N4: potential use of null pointer ```cpp bool DDraw_grDispatcher::Finit() { .... ddobj_ -> SetCooperativeLevel((HWND)Get_hWnd(),DDSCL_NORMAL); if(fullscreen_ && ddobj_) ddobj_ -> RestoreDisplayMode(); .... } ``` The PVS\-Studio warning: [V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'ddobj\_' pointer was utilized before it was verified against nullptr\. Check lines: 211, 212\. ddraw\_gr\_dispatcher\.cpp 211 The _ddobj_ pointer is dereferenced for the function call\. There's also a check for the pointer below\. Based on this check, the analyzer deduced that the _ddobj_ variable may contain _nullptr_ and warns us about it\. We can rewrite the code as follows: ```cpp if (ddobj_) { ddobj_ -> SetCooperativeLevel((HWND)Get_hWnd(),DDSCL_NORMAL); if(fullscreen_) ddobj_ -> RestoreDisplayMode(); } ``` ### Warning N5: destructor is not virtual ```cpp template class PtrHandle { .... ~PtrHandle() { delete ptr; } .... private: T *ptr; }; ``` The PVS\-Studio warning: [V599](https://pvs-studio.com/en/docs/warnings/v599/) Instantiation of PtrHandle < ResourceUser \>: The virtual destructor is not present, although the 'ResourceUser' class contains virtual functions\. Handle\.h 14 There's no error in the code of the _PtrHandle_ class\. However, the issue is that it stores an object of the _ResourceUser_ type\. Let's look at the class declaration: ```cpp class ResourceUser { int ID; static int IDs; public: ResourceUser(time_type period) { dtime = period; time = 0; ID = ++IDs; } virtual int quant() { return 1; } protected: time_type time; time_type dtime; virtual void init_time(time_type time_) { time = time_ + time_step(); } virtual time_type time_step() { return dtime; } friend class ResourceDispatcher; }; ``` The class contains virtual functions\. So, other classes are expected to be inherited from this class\. In such a case, the destructor should be declared as virtual\. Otherwise, the _PtrHandle_ class destructor won't completely destroy objects that are inherited from _ResourceUser_\. This leads to undefined behavior and resource leaks\. ### Warning N6: invalid delete operator ```cpp char* grDispatcher::temp_buffer(int size) { if(size <= 0) size = 1; if(size > temp_buffer_size_){ delete temp_buffer_; temp_buffer_ = new char[size]; temp_buffer_size_ = size; } return temp_buffer_; } ``` The PVS\-Studio warning: [V611](https://pvs-studio.com/en/docs/warnings/v611/) The memory was allocated using the 'operator new\[\]' but was released using the 'operator delete'\. The 'delete\[\] temp\_buffer\_;' statement should be used instead\. Check lines: 1241, 1242\. gr\_dispatcher\.cpp 1241 The _temp\_buffer\__ variable stores pointers to arrays created using the operator _new\[\]_\. So, the operator _delete\[\]_ should destroy them\. In the code we're looking at, _delete_, which is designed for single objects, is used to destroy an array\. The code results in undefined behavior\. Note\. Someone may argue that everything will work because the array consists of _char_\-type items and the operators _new_/_delete_ only call the _malloc_/_free_ functions\. [That's not true](https://pvs-studio.com/en/blog/posts/cpp/0973/)\. The implementation of operators can be quite diverse\. For example, individual objects and arrays can be created in different pre\-allocated \(reserved\) memory pools for optimization purposes\. ### Warning N7: wrong variable type for synchronization ```cpp static int b_thread_must_stop=0; void MpegDeinitLibrary() { .... if (hThread!=INVALID_HANDLE_VALUE) { b_thread_must_stop=1; while(b_thread_must_stop==1) Sleep(10); } .... } ``` The PVS\-Studio warning: [V712](https://pvs-studio.com/en/docs/warnings/v712/) Be advised that compiler may delete this cycle or make it infinity\. Use volatile variable\(s\) or synchronization primitives to avoid this\. PlayOgg\.cpp 293 A global variable of unsafe type is used to synchronize threads\. This is a bad idea, as the compiler, during optimization, may decide that the variable doesn't change and create an infinite loop\. Also, the change in the variable may not be atomic\. In such a case, replace the _int_ type with [_atomic\_int_](https://en.cppreference.com/w/cpp/atomic/atomic): ```cpp #include static atomic_int b_thread_must_stop { 0 }; ``` ### Warning N8: memory leak ```cpp bool qdAnimationMaker::insert_frame(....) { // IMPORTANT(pabdulin): auto_ptr usage was removed qdAnimationFrame* fp = new qdAnimationFrame; fp -> set_file(fname); fp -> set_length(default_frame_length_); if (!fp -> load_resources()) return false; .... delete fp; return true; } ``` The PVS\-Studio warning: [V773](https://pvs-studio.com/en/docs/warnings/v773/) The function was exited without releasing the 'fp' pointer\. A memory leak is possible\. qd\_animation\_maker\.cpp 40 If a resource loading error occurs, the function returns early\. The object whose pointer is stored in the _fp_ variable isn't deleted\. That's a memory leak\. Here's the fixed code: ```cpp if (!fp -> load_resources()) { delete fp; return false; } ``` However, even after fixing the error, we can't call the code good\. As we can see here, manual memory management causes errors\. It's much better to use smart pointers\. Here's an interesting thing\. Take a look at the comment: ```cpp // IMPORTANT(pabdulin): auto_ptr usage was removed ``` Maybe the code was correct once, as long as it used a smart pointer of the [_std::auto\_ptr_](https://en.cppreference.com/w/cpp/memory/auto_ptr) type\. Since then, the class has become obsolete\. Instead of replacing it with [_std::unique\_ptr_](https://en.cppreference.com/w/cpp/memory/unique_ptr), the developer decided to work with pointers "in manual mode" but failed\. ### Warning N9: false positive ```cpp class PtrHandle { .... PtrHandle& operator=(PtrHandle& p) { if (get() != p.get()) { delete ptr; ptr = p.release(); } return *this; } .... T* get() const { return ptr; } .... private: T *ptr; }; ``` The PVS\-Studio warning: [V794](https://pvs-studio.com/en/docs/warnings/v794/) The assignment operator should be protected from the case of 'this \=\= &p'\. Handle\.h 19 This is the only one of the 10 warnings selected by the analyzer that has turned out to be false\. The [V794](https://pvs-studio.com/en/docs/warnings/v794/) diagnostic rule warns that the object should be protected from being copied into itself\. The analyzer looks for the following construct in the function body: ```cpp if (this != &p) ``` However, developers have written a more complex check here using the _get_ function call: ```cpp if (get() != p.get()) ``` Perhaps the code author wanted to create protection not only against copying a smart pointer to itself, but also against copying two different smart pointers referring to the same object\. That doesn't make any sense, though\. If two smart pointers \(without counting references\) refer to the same object, that's bad\. A double destruction of the object is possible\. So, such a check is really no better\. It only further confuses the analyzer and the programmers reading the code\. Well, the analyzer didn't understand what was going on\. To fix the false positive, we can do one of the following things\. **Option 1\.** Add a comment to suppress the warning\. ```cpp PtrHandle& operator=(PtrHandle& p) { //-V794 if (get() != p.get()) ``` The comment explicitly informs the PVS\-Studio analyzer that the code is safe\. **Option 2\.** Rewrite the check\. ```cpp if (this != &p) ``` I prefer this option over the other\. The option with function calls looks heavy in my opinion\. If we want to have more control, we can add a check like this one: ```cpp PtrHandle& operator=(PtrHandle& p) { if (this != &p) { if (get() == p.get()) { // It's bad. One object in two smart pointers. // It's debugging time. assert(false); throw std::logic_error("Multiple ownership of an object."); } delete ptr; ptr = p.release(); } return *this; } ``` The check helps detect cases when two smart pointers suddenly control the same object\. ### Warning N10: re\-assignment ```cpp bool qdGameObjectMoving::update_screen_pos() { .... if(get_cur_state() -> state_type() == qdGameObjectState::STATE_WALK){ qdGameObjectStateWalk::OffsetType offs_type= qdGameObjectStateWalk::OFFSET_WALK; // <= switch(movement_mode_){ case MOVEMENT_MODE_STOP: offs_type = qdGameObjectStateWalk::OFFSET_STATIC; break; case MOVEMENT_MODE_TURN: offs_type = qdGameObjectStateWalk::OFFSET_STATIC; break; case MOVEMENT_MODE_START: offs_type = qdGameObjectStateWalk::OFFSET_START; break; case MOVEMENT_MODE_MOVE: offs_type = qdGameObjectStateWalk::OFFSET_WALK; // <= break; case MOVEMENT_MODE_END: offs_type = qdGameObjectStateWalk::OFFSET_END; break; } offs += static_cast( get_cur_state()) -> center_offset(direction_angle_, offs_type); } .... } ``` The PVS\-Studio warning: [V1048](https://pvs-studio.com/en/docs/warnings/v1048/) The 'offs\_type' variable was assigned the same value\. qd\_game\_object\_moving\.cpp 1094 The analyzer issues the warning because in the _MOVEMENT\_MODE\_MOVE_ branch, the _offs\_type_ variable is assigned a value that is already in it\. Technically, the analyzer is correct\. Re\-assignment makes no sense\. Also, sometimes it's not just extra code, but an error\. In this case, there's no bug\. Let's take a look at how we can get rid of the warning\. **Option 1\.** Add the _//\-V1048_ comment to suppress the warning\. **Option 2\.** Rewrite the code a bit by adding the _default_ branch: ```cpp if(get_cur_state() -> state_type() == qdGameObjectState::STATE_WALK){ qdGameObjectStateWalk::OffsetType offs_type; switch(movement_mode_){ case MOVEMENT_MODE_STOP: offs_type = qdGameObjectStateWalk::OFFSET_STATIC; break; case MOVEMENT_MODE_TURN: offs_type = qdGameObjectStateWalk::OFFSET_STATIC; break; case MOVEMENT_MODE_START: offs_type = qdGameObjectStateWalk::OFFSET_START; break; case MOVEMENT_MODE_MOVE: offs_type = qdGameObjectStateWalk::OFFSET_WALK; break; case MOVEMENT_MODE_END: offs_type = qdGameObjectStateWalk::OFFSET_END; break; default: offs_type = qdGameObjectStateWalk::OFFSET_WALK; } offs += static_cast( get_cur_state()) -> center_offset(direction_angle_, offs_type); } ``` The warning is gone, but I don't like the fact that the _offs\_type_ variable is now not initialized where it's declared\. Let's take another step in refactoring and move some of the code into a separate function\. ```cpp qdGameObjectStateWalk::OffsetType qdGameObjectMoving::GetOffsetType() { switch(movement_mode_){ case MOVEMENT_MODE_STOP: return qdGameObjectStateWalk::OFFSET_STATIC; case MOVEMENT_MODE_TURN: return qdGameObjectStateWalk::OFFSET_STATIC; case MOVEMENT_MODE_START: return qdGameObjectStateWalk::OFFSET_START; case MOVEMENT_MODE_MOVE: return qdGameObjectStateWalk::OFFSET_WALK; case MOVEMENT_MODE_END: return qdGameObjectStateWalk::OFFSET_END; } return qdGameObjectStateWalk::OFFSET_WALK; } .... if(get_cur_state() -> state_type() == qdGameObjectState::STATE_WALK){ qdGameObjectStateWalk::OffsetType offs_type = GetOffsetType(); offs += static_cast( get_cur_state()) -> center_offset(direction_angle_, offs_type);} .... ``` The code is now shorter and easier to read\. Cool\. Also, we don't need the _offs\_type_ variable at all now, and we can shorten the code a bit\. ```cpp if(get_cur_state() -> state_type() == qdGameObjectState::STATE_WALK){ offs += static_cast( get_cur_state()) -> center_offset(direction_angle_, GetOffsetType()); } ``` This is a good example: the analyzer warning prompted us to refactor the code to make it simpler and better\. ## Conclusion The article covered the following topics: 1. How can the **Best** button help when you start using the PVS\-Studio analyzer for the first time? I suggest you [get a trial version](https://pvs-studio.com/en/pvs-studio/try-free/) and try to analyze your projects\. 1. What errors does PVS\-Studio help you find and fix at the coding stage? The sooner an error is found, the easier and less expensive it is to fix it\. 1. How do the analyzer warnings encourage you to enhance \(refactor\) your code? In future articles about qdEngine, I'll cover other errors found in the project\. Thank you for reading\. ## Additional links 1. [C\+\+ tools evolution: static code analyzers](https://pvs-studio.com/en/blog/posts/cpp/0873/) 1. [How warnings simplify your code](https://pvs-studio.com/en/blog/posts/cpp/0968/) 1. [Static analyzer nudges you to write clean code](https://pvs-studio.com/en/blog/posts/cpp/1115/) 1. [Virtual function calls in constructors and destructors \(C\+\+\)](https://pvs-studio.com/en/blog/posts/cpp/0891/) 1. [How to introduce a static code analyzer in a legacy project and not to discourage the team](https://pvs-studio.com/en/blog/posts/0743/)