Let's check the qdEngine game engine, part one: top 10 warnings issued by PVS-Studio
Tags: cpp, gamedev, programming
Published: 2024-04-27 11:27:10

Author: Andrey Karpov

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\.

![](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\.

![](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\.

<spoiler title="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\.

</spoiler>


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/?utm_source=website&utm_medium=habr&utm_campaign=article&utm_content=1119) 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<qdGameObjectStatic*>(*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<qdGameObjectStatic*>(*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 T>
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 <atomic>
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<qdGameObjectStateWalk*>(
      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;
  defaule:
    offs_type = qdGameObjectStateWalk::OFFSET_WALK;
  }

  offs += static_cast<qdGameObjectStateWalk*>(
    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 return qdGameObjectStateWalk::OFFSET_WALK;
}
....
if(get_cur_state() -> state_type() == qdGameObjectState::STATE_WALK){
  qdGameObjectStateWalk::OffsetType offs_type = GetOffsetType();
  offs += static_cast<qdGameObjectStateWalk*>(
    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<qdGameObjectStateWalk*>(
    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/?utm_source=website&utm_medium=habr&utm_campaign=article&utm_content=1119) 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/)