# Heroes of Code and Magic: VCMI game engine analysis

Sometimes you might wish to feel nostalgic and play your favorite old game, but certain things in such games may seem outdated\. So, to breathe new life into an old project, some enthusiasts set the goal to recreate and improve its source code\. We decided to use the PVS\-Studio static analyzer and check how VCMI developers cope with this task\.

![1058_VCMI/image1.png](https://import.viva64.com/docx/blog/1058_VCMI/image1.png)

## Briefly about the project

VCMI Project is an open\-source game engine for Heroes of Might and Magic 3\. The VCMI engine is cross\-platform and runs on Windows, Linux, Android, macOS, and iOS\. The engine got an update, resulting in improved animations, increased performance, the updated graphical interface, fixed bugs, the improved mod system, and much more\. You can find detailed information about the project [here](https://vcmi.eu/)\. 

A large team of developers is regularly updating the project, and the number of new code lines is constantly increasing\. In such circumstances, it is not a miracle that errors may occur in code\. No matter how experienced a programmer is, mistakes can appear because of inattention or fatigue\. Moreover, VCMI developers work on a non\-commercial basis in their spare time\.

The purpose of the article is not to offend developers\. We just want to show how useful static analysis tools can be\. They can greatly simplify developers' life and save users from possible bugs\. This will not only reduce time for development and testing, but also help you avoid mistakes in the final product, which can spoil the pleasure when using it\.

It's easier to use a static analyzer than it seems\. For example, PVS\-Studio provides a [free version](https://pvs-studio.com/en/blog/posts/0614/) for open\-source projects, and in the [documentation](https://pvs-studio.com/en/docs/) you can see how to quickly integrate it into the development process\.

Fans of the Heroes of Might and Magic series may also be interested in our [article about Free Heroes of Might and Magic II](https://pvs-studio.com/en/blog/posts/cpp/0804/)\.

Let's check how effective static analysis is and examine some code snippets which triggered the analyzer\.

![1058_VCMI/image2.png](https://import.viva64.com/docx/blog/1058_VCMI/image2.png)

## Check results

Before we proceed with the analysis, it is worth mentioning that the code is of high quality\. In some places, developers even inserted superfluous checks to be on the safe side\. Nevertheless, a lot of errors got into the code\.

The analysis was carried out on the [10fc6ce](https://github.com/vcmi/vcmi/tree/10fc6cecef54d3f6a952e4f6c5d794160322f61e) commit\.

### Memory leaks

The issue with memory in games is acute\. Of course, nowadays Heroes of Might and Magic 3 is hardly a demanding game that runs at the limit of the available RAM\. However, it is extremely unpleasant to see that some game takes up a few extra gigabytes from RAM in a couple of hours\. Needless to say, no one likes memory leaks\. Let's consider the following code fragments and analyzer warnings\.

**Fragment N1**

```cpp
CGObjectInstance *CMapLoaderH3M::readDwellingRandom(....)
{
  auto *object = new CGDwelling();
  CSpecObjInfo *spec = nullptr;

  switch(objectTemplate->id)
  {
  case Obj::RANDOM_DWELLING:
    spec = new CCreGenLeveledCastleInfo();
    break;
  case Obj::RANDOM_DWELLING_LVL:
    spec = new CCreGenAsCastleInfo();
    break;
  case Obj::RANDOM_DWELLING_FACTION:
    spec = new CCreGenLeveledInfo();
    break;
  default:
    throw std::runtime_error("Invalid random dwelling format");
  }

  spec->owner = object;

  ....

  object->info = spec;
  return object;
}
```

PVS\-Studio warning: V773 The exception was thrown without releasing the 'object' pointer\. A memory leak is possible\. MapFormatH3M\.cpp 1173

Here we see that if we get into the _default_ branch of the _switch_ statement, an exception will be thrown\. In this case, the memory allocated by the _operator new_ for the _object_ pointer will leak\. 

To solve this problem, you can move the declaration of the _object_ pointer after the body of the _switch_ statement, but it is much better to use smart pointers\. Here is a possible fix:

```cpp
std::unique_ptr<CGDwelling> CMapLoaderH3M::readDwellingRandom(....)
{
  std::unique_ptr<CSpecObjInfo> spec{ nullptr };
  
  switch(objectTemplate->id)
  {
  case Obj::RANDOM_DWELLING:
    spec = std::make_unique<CCreGenLeveledCastleInfo>();
    break;
  case Obj::RANDOM_DWELLING_LVL:
    spec = std::make_unique<CCreGenAsCastleInfo>();
    break;
  case Obj::RANDOM_DWELLING_FACTION:
    spec = std::make_unique<CCreGenLeveledInfo>();
    break;
  default:
    throw std::runtime_error("Invalid random dwelling format");
  }
  
  auto object = std::make_unique<CGDwelling>();
  spec->owner = object.get();

  ....

  object->info = std::move(spec);
  return object;
}
```

**Fragment N2**

```cpp
CTownHandler::CTownHandler():
  randomTown(new CTown()),
  randomFaction(new CFaction())
{
  randomFaction->town = randomTown;
  randomTown->faction = randomFaction;
  randomFaction->identifier = "random";
  randomFaction->modScope = "core";
}

CTownHandler::~CTownHandler()
{
  delete randomTown;
}
```

PVS\-Studio warning: V773 The 'randomFaction' pointer was not released in destructor\. A memory leak is possible\. CTownHandler\.cpp 282

In the member initializer list of the constructor, two pointers are initialized with the _operator new_, but in the destructor, memory is freed only by one pointer\.

In the simplest case, the fixed code looks as follows:

```cpp
CTownHandler::~CTownHandler()
{
  delete randomFaction;
  delete randomTown;
}
```

However, you can replace simple pointers with smart ones \(for example, with _std::unique\_ptr_\) and forget about such issues forever\.

![1058_VCMI/image3.png](https://import.viva64.com/docx/blog/1058_VCMI/image3.png)

**Fragment N3**

```cpp
template <class T>
void addModificator()
{
  modificators.emplace_back(new T(*this, map, generator));
}
```

PVS\-Studio warning: V1023 A pointer without owner is added to the 'modificators' container by the 'emplace\_back' method\. A memory leak will occur in case of an exception\. Zone\.h 122

Here the situation is more interesting\. The _modificators_ container is a doubly linked list of smart _std::unique\_ptr_ pointers\. Developers want to insert a new smart pointer "in\-place" at the end of the list with the help of the _emplace\_back _function and pass a raw pointer to an object of the _T_ type that the _operator new_ created\.

Such code contains a potential problem\. What if the _std::bad\_alloc_ exception will be thrown when a new node is created in _std::list_? That's right, here you will get a memory leak\.

It's easy to fix the issue — use _std::make\_unique_ and replace _emplace\_back_ with _push\_back_:

```cpp
template <class T>
void addModificator()
{
  modificators.push_back(std::make_unique<T>(*this, map, generator));
}
```

Yes, an extra move constructor will be called in this code, but we eliminate the possibility of a memory leak\.

The reader may also ask: "Why was it necessary to replace _emplace\_back_ with _push\_back_"? The _push\_back_ function has a great advantage — it creates less work for the compiler, and therefore, less time is spent on compilation\.

When you call _push\_back_, the compiler only has to select an overload\. When you call _emplace\_back_, the compiler will both instantiate the function template and select an overload\. So, both these functions will do the same\. But this does not mean that we should always give priority to _push\_back_\. Use _emplace\_back_ when you can pass arguments to the constructor of the container element type\. Otherwise, if your object has already been created or constructor arguments cannot be passed, the _push\_back_ function is more preferable\.

### Undefined behavior

In this section, we will look at code fragments that lead to undefined behavior\. For those who are interested in UB, I also recommend the following [article](https://pvs-studio.com/en/blog/posts/cpp/1024/)\. Let's move on to the analyzer's warnings\.

**Fragment N4**

```cpp
void ApplyGhNetPackVisitor::visitTradeOnMarketplace(....)
{
  ....
  bool allyTownSkillTrade = (....
                          && gh.getPlayerRelations(player, hero->tempOwner) 
                          && ....);
  if (hero && ....)
    gh.throwAndComplain(&pack, "This hero can't use this marketplace!");
  ....
}
```

PVS\-Studio warning: V595 The 'hero' pointer was utilized before it was verified against nullptr\. Check lines: 182, 184\. NetPacksServer\.cpp 182

Here we see that the _hero_ pointer is dereferenced before a check for _nullptr_\. A similar warning:

* V595 The 'gs' pointer was utilized before it was verified against nullptr\. Check lines: 628, 629\. Client\.cpp 628

**Fragment N5**

```cpp
void Queries::popIfTop(QueryPtr query)
{
  //LOG_TRACE_PARAMS(logGlobal, "query='%d'", query);
  if(!query)
    logGlobal->error("The query is nullptr! Ignoring.");
  popIfTop(*query);
}
```

PVS\-Studio warning: V1004 The 'query' pointer was used unsafely after it was verified against nullptr\. Check lines: 246, 249\. CQuery\.cpp 249

This case is more interesting\. Here, developers log an error to handle the situation when _query_ equals _nullptr_\. However, after that, the program will continue execution, and the null pointer will be dereferenced, which leads to [undefined behavior](https://pvs-studio.com/en/blog/posts/cpp/0306/)\. Perhaps, it's better to interrupt the execution of the function after logging\.

**Fragment N6**

```cpp
void CCallback::trade(....)
{
  ....
  pack.marketId = dynamic_cast<const CGObjectInstance *>(market)->id;
  ....
}
```

PVS\-Studio warning: V522 There might be dereferencing of a potential null pointer\. CCallback\.cpp 255

Here the result of the _dynamic\_cast_ operator is dereferenced\. Since _dynamic\_cast_ can return _nullptr_, _nullptr_ may be dereferenced\. 

The analyzer issued 112 warnings of the V522 diagnostic rule, and 100 of these warnings were associated with _dynamic\_cast_\. For every fifth warning, the code had a check for the resulting pointer using the _assert_ macro, but this is not a panacea\. The _assert_ macro expands to an empty string in the release version\. Therefore, we can say that no _dynamic\_cast_ result was checked\.

You can say that the developers were confident in the returned result\. However, we should remember that tomorrow the code may change, and suddenly _dynamic\_cast_ will start to return _nullptr_\. The cost of an error in this case will be much more than an extra check\.

Here is just a part of such warnings:

* V522 There might be dereferencing of a potential null pointer 'boat'\. MapRendererContext\.cpp 47
* V522 There might be dereferencing of a potential null pointer 'hero'\. MapRendererContext\.cpp 134
* V522 There might be dereferencing of a potential null pointer 'hero'\. MapViewController\.cpp 291
* V522 There might be dereferencing of a potential null pointer\. CArtifactsOfHeroAltar\.cpp 102
* V522 There might be dereferencing of a potential null pointer 'boat'\. MapViewController\.cpp 323
* V522 There might be dereferencing of a potential null pointer 'hero'\. MapViewController\.cpp 333
* V522 There might be dereferencing of a potential null pointer 'dst'\. NetPacksClient\.cpp 902
* V522 There might be dereferencing of a potential null pointer 'questObj'\. AIMovementAfterDestinationRule\.cpp 130
* V522 There might be dereferencing of a potential null pointer 'quest'\. AIUtility\.cpp 258
* V522 There might be dereferencing of a potential null pointer 'd'\. AIUtility\.cpp 397
* \.\.\.

### Unreachable code

In this section, I'd like to consider unreachable code — code that will not be executed under any conditions in the program\. Most often, such cases appear due to violations in the conditions, but there are cases when inattention leads to them\.

![1058_VCMI/image4.png](https://import.viva64.com/docx/blog/1058_VCMI/image4.png)

So, here they are from ~~left to right~~\.

**Fragment N7**

```cpp
size_t CTrueTypeFont::getGlyphWidth(const char *data) const
{
  if (....)
    return fallbackFont->getGlyphWidth(data);

  return getStringWidth(....);
  int advance;
  TTF_GlyphMetrics(....);
  return advance;
}
```

PVS\-Studio warning: V779 Unreachable code detected\. It is possible that an error is present\. CTrueTypeFont\.cpp 86

It's better to pay attention to this function: the code after _return getStringWidth\(\.\.\.\.\);_ will never be executed\.

**Fragment N8**

```cpp
CConnection::CConnection (....)
{
  ....
  while (endpoint_iterator != end)
  {
    ....
    if (!error)
    {
      init();
      return;
    }
    else
    {
      throw std::runtime_error(....);
    }
    endpoint_iterator++;
  }
}
```

PVS\-Studio warning: V779 Unreachable code detected\. It is possible that an error is present\. Connection\.cpp 115

This fragment looks strange too\. Developers try to iterate to a certain boundary, but the line with _endpoint\_iterator\+\+_ will never be executed\. I could not figure out the author's idea, perhaps one of the readers will succeed\.

**Fragment N9**

```cpp
void CCreatureHandler::loadStackExp (....)
{
  ....
  switch (mod[0])
  {
    ....
    case 'D':
      b.type = BonusType::ADDITIONAL_ATTACK; break;
    case 'f':
      b.type = BonusType::FEARLESS; break;
    case 'F':
      b.type = BonusType::FLYING; break;
    case 'm':
      b.type = BonusType::MORALE; break;         // <=
      b.val = 1;
      b.valType = BonusValueType::INDEPENDENT_MAX;
      break;
    ....
  }
}
```

PVS\-Studio warning: V779 Unreachable code detected\. It is possible that an error is present\. CCreatureHandler\.cpp 1094

In _case 'm'_, a code part will not be executed\. As we see, the code lines above are very similar\. We can conclude that this error appeared as a result of copy\-paste\.

**Fragment N10**

```cpp
bool Animation::loadFrame(size_t frame, size_t group)
{
  ....
  if (....)
  {
    if (defFile)
    {
      auto frameList = defFile->getEntries();

      if (....)
      {
        ....
        return true;
      }
    }
    return false;
    // still here? image is missing

    printError(frame, group, "LoadFrame");
    images[group][frame] = std::make_shared<QImage>("DEFAULT");
  }
  ....
}
```

PVS\-Studio warning: V779 Unreachable code detected\. It is possible that an error is present\. Animation\.cpp 545

In this fragment, everything that is after _return false;_ will not be executed\. Judging by the comment in the code, the developers are aware of this, and such code is only a temporary measure\.

The following case is the most interesting in my opinion\.

**Fragment N11**

```cpp
std::string CComponent::getSubtitleInternal()
{
  ....
  if (val)
    return ....;
  else
    return val > 1 ? creature->getNamePluralTranslated()
                   : creature->getNameSingularTranslated();
  ....
}
```

PVS\-Studio warning: V547 Expression 'val \> 1' is always false\. CComponent\.cpp 218

The control flow can get into the _else_ branch only if _val \=\= 0_\. Accordingly, _val \> 1_ will never return _true,_ and _creature\-\>getNamePluralTranslated\(\)_ is unreachable code\.

It is worth noting that the developers left a comment that this function should be fixed\. However, they either did not have time to do it \(by the time when the article was written\), or they forgot\. In any case, with a static analyzer one will notice such an error when writing a function and immediately correct it\.

**Fragment N12**

```cpp
void deallocate_segment(....) 
{
  ....
  if (seg_index >= first_block) 
  {
    segment_element_allocator_traits::deallocate(....);
  }
  else 
  if (seg_index == 0) 
  {
    elements_to_deallocate = first_block > 0 ? this->segment_size(first_block) 
                                             : this->segment_size(0);
    ....
  }
}
```

PVS\-Studio warning: V547 Expression 'first\_block \> 0' is always true\. concurrent\_vector\.h 669

Let's figure out what's going on here\. If the first check fails, then _seg\_index < first\_block_\. OK, let's keep this in mind\. Now, if _seg\_index \=\= 0_, then _first\_block \> 0_\. After that, we again check that _first\_block \> 0_, and the analyzer indicates that this condition is always true\. Therefore, the _this\-\>segment\_size\(0\)_ expression will never be executed\. It may be difficult for a person to trace such a logical chain, but this is easy for a soulless machine\.

The symbolic execution technology in the analyzer detects such errors\. If you are interested what it is and how it works, I suggest you refer to this article: [PVS\-Studio Static Code analysis technologies](https://pvs-studio.com/en/blog/posts/0908/)\.

### Operator overloading error

In this section, we will look at a more conceptual error when the assignment operator is incorrectly overloaded\.

**Fragment N13**

```cpp
ObjectTemplate & ObjectTemplate::operator=(const ObjectTemplate & rhs)
{
  ....
  width = rhs.width;
  height = rhs.height;
  visitable = rhs.visitable;
  blockedOffsets = rhs.blockedOffsets;
  blockMapOffset = rhs.blockMapOffset;
  visitableOffset = rhs.visitableOffset;
  usedTiles.clear();
  usedTiles.resize(rhs.usedTiles.size());
  for(size_t i = 0; i < usedTiles.size(); i++)
    std::copy(....);
  return *this;
}
```

PVS\-Studio warning: V794 The assignment operator should be protected from the case of 'this \=\= &rhs'\. ObjectTemplate\.cpp 88

The analyzer tells us that an object cannot be assigned to itself\. When the _usedTiles\.clear\(\)_ function is called, we will erase the _usedTiles_ data member of our object\. As a result, _resize_ will make the container size equal to 0, and nothing will be copied \(and there is nothing left to copy\)\. It turns out that a part of the object will be lost\.

You can solve the problem in one of the following ways:

* Add the _this \=\= &rhs_ check and exit the function in advance\.
* Implement _operator\=_ using the [copy\-and\-swap](https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Copy-and-swap) idiom\.

## Questionable code fragments

This section contains warnings that cannot be called errors, because they do not lead to serious consequences\. Some of the fragments may be an author's idea, which only the author can confirm; while some fragments are unnecessary actions that are better avoided\. 

### Unnecessary condition checks

Let's move on to situations where developers made redundant checks either because of logical expressions that were incorrectly calculated or as a safety net\. 

**Fragment N14**

```cpp
void BattleStacksController::updateBattleAnimations(uint32_t msPassed)
{
  ....
  if (hadAnimations && currentAnimations.empty())
  {
    //stackAmountBoxHidden.clear();
    owner.executeStagedAnimations();
    if (currentAnimations.empty())
      owner.onAnimationsFinished();
  }
  ....
}
```

PVS\-Studio warning: V547 Expression 'currentAnimations\.empty\(\)' is always true\. BattleStacksController\.cpp 384

The nested check is always true\. Perhaps it was necessary to check another container or the check can even be removed\.

**Fragment N15**

```cpp
std::vector<BattleHex> CStack::meleeAttackHexes(....)
{
  ....
  int mask = 0;
  ....
  if (....) 
  {
    if ((mask & 1) == 0)
    {
      mask |= 1;
      res.push_back(defenderPos);
    }
  }
  ....
}
```

PVS\-Studio warning: V547 Expression '\(mask & 1\) \=\= 0' is always true\. CStack\.cpp 262

The _mask_ variable is declared equal to zero and then does not change in code till the check\. The mask was probably modified earlier, but it does not change after refactoring\.

**Fragment N16**

```cpp
bool CGarrisonSlot::mustForceReselection() const
{
  .... 
  if (!creature || !selection->creature)
    return false;
  ....
  if (!owner->removableUnits)
  {
    if (selection->upg == EGarrisonType::UP)
      return true;
    else
      return creature || upg == EGarrisonType::UP;
  }
}
```

PVS\-Studio warning: V560 A part of conditional expression is always true: creature\. CGarrisonInt\.cpp 284

At the very beginning of the code fragment, there was already the _if \(\!creature\)_ check and the exit from the function\. Accordingly, the last _return_ will always return _true_\.

**Fragment N17**

```cpp
std::optional<BattleAction> CBattleAI::considerFleeingOrSurrendering()
{
  ....
  if (!bs.canFlee || !bs.canSurrender)
  {
    return std::nullopt;
  }
  auto result = cb->makeSurrenderRetreatDecision(bs);
  if (!result && bs.canFlee && bs.turnsSkippedByDefense > 30)
  {
    return BattleAction::makeRetreat(bs.ourSide);
  }
  ....
}
```

PVS\-Studio warning: V560 A part of conditional expression is always true: bs\.canFlee\. BattleAI\.cpp 837

It is similar to the previous one, although it looks more like a safety net if the _bs\.canFlee_ data member changes after the first check \(although it cannot change during the _cb\-\>makeSurrenderRetreatDecision\(bs\)_ function call\)\.

A similar warning:

* V547 Expression 'hero\-\>movement \> 0' is always true\. ExecuteHeroChain\.cpp 140

**Fragment N18**

```cpp
void ApplyOnServerNetPackVisitor::visitLobbySetCampaign(LobbySetCampaign & pack)
{
  ....
  if (   !isCurrentMapConquerable
      || (isCurrentMapConquerable && i == *pack.ourCampaign->currentMap))
  {
    srv.setCampaignMap(i);
  }
  ....
}
```

PVS\-Studio warning: V728 An excessive check can be simplified\. The '\|\|' operator is surrounded by opposite '\!isCurrentMapConquerable' and 'isCurrentMapConquerable' expressions\.  NetPacksLobbyServer\.cpp 225

Here you can notice that the check can be simplified to the following:

```cpp
if (!isCurrentMapConquerable || i == *pack.ourCampaign->currentMap)
```

### More

This section contains the rest of the exciting analyzer warnings, which are difficult to categorize by types of errors considered above\.

**Fragment N19**

```cpp
template <typename Handler>
void serialize(Handler &h, const int version)
{
  h & x;
  h & y;
  h & w;
  h & h; 
}
```

PVS\-Studio warning: V501 There are identical sub\-expressions to the left and to the right of the '&' operator: h &h Rect\.h 158

This is a member function of the _Rect_ class, which contains these data members: _x, y, w, h_\. Obviously, the _operator &_ is overloaded\. However, it is not possible to see how, since the function is a template\. In this case, there is an object of the _Handler_ type to the left and right of the _&_ operator, but not the _h_ data member of the _Rect_ class\. So, it is difficult to say for sure whether it is intended or not\. 

For example, the _AIUtility\.h_ file contains the following function on line 74:

```cpp
template<typename Handler>
void serialize(Handler & h, const int version)
{
  h & this->h;
  h & hid;
  h & name;
}
```

It explicitly specifies the _this\-\>h_ data member\. I reviewed a couple of such functions \(there are many of them\), and there is no object of the _Handler_ type that is simultaneously used with the _operator &_ on the left and on the right\.

**Fragment N20**

```cpp
void CArmedInstance::updateMoraleBonusFromArmy()
{
  ....
  //-1 modifier for any Undead unit in army
  const ui8 UNDEAD_MODIFIER_ID = -2;
  ....
}
```

PVS\-Studio warning: V569 Truncation of constant value \-2\. The value range of unsigned char type: \[0, 255\]\. CArmedInstance\.cpp 123

The UNDEAD\_MODIFIER\_ID constant has the _unsigned char_ type\. The analyzer warns that the variable will be truncated\. As a result of this assignment, the _UNDEAD\_MODIFIER\_ID_ variable will be equal to 254\. Perhaps this is what the author of the code intended\. However, it is better to replace the value with a more meaningful one for better code readability\.

**Fragment N21**

```cpp
void CGameHandler::endBattleConfirm(....)
{
  ....
  for (int i = 0; i < cs.spells.size(); i++)
  {
    names << "%s";
    if (i < cs.spells.size() - 2)
      names << ", ";
    else if (i < cs.spells.size() - 1)
      names << "%s";
  }
  ....
}
```

PVS\-Studio warning: V658 A value is being subtracted from the unsigned variable\. This can result in an overflow\. In such a case, the '<' comparison operation can potentially behave unexpectedly\. Consider inspecting the 'i < cs\.spells\.size\(\) \- 2' expression\. CGameHandler\.cpp 760

Here_ cs\.spells\.size\(\)_ returns a value of the_ size\_t_ type\. If the _cs\.spells_ container contains only 1 object, then an overflow will occur in the _cs\.spells\.size\(\) – 2_ expression, and _i_ will be compared with the maximum number that can be stored in _size\_t_\. Therefore, "%s, " will be written in the stream\.

Perhaps this was intended, or developers are sure that there is always more than 1 object in the container, but the code looks peculiar\.

And the last case:

**Fragment N22**

```cpp
CGameState::CrossoverHeroesList
CGameState::getCrossoverHeroesFromPreviousScenarios()
  const
{
  ....
  crossoverHeroes.heroesFromAnyPreviousScenarios =
  crossoverHeroes.heroesFromPreviousScenario = heroes;
  crossoverHeroes.heroesFromPreviousScenario = heroes;
  ....
}
```

PVS\-Studio warning: V519 The variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 1203, 1204\. CGameState\.cpp 1204

Here _heroes_ are assigned to _crossoverHeroes\.heroesFromPreviousScenario_ twice in a row\. Perhaps it's better to save _heroes_ in another variable\.

## Conclusion

The article does not contain the entire list of detected errors, as some of them were in third\-party libraries\. Although such errors are still in VCMI, I did not consider them in the article, because VCMI developers are not directly related to them\.

To prevent such warnings, PVS\-Studio allows you to [exclude selected directories or files from the analysis](https://pvs-studio.com/en/docs/manual/6640/), and that's what I did\. I found that the developers checked the project with the Coverity analyzer, but the information about the last check refers to April 14, 2018\. Unfortunately, we don't know whether the developers are using any static analyzer now\.

We can say that the [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) solution would help avoid many dangerous and dubious places in code and, thus, protect players from possible bugs in one of the best strategies of the 20th century\.