Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
Webinar: C++ semantics - 06.11
Recently we found out that the new version of the fheroes2 project was released. In our company there are many fans of Heroes of Might and Magic game series. So, we couldn't pass it up and checked the project by PVS-Studio.
Free Heroes of Might and Magic II is an open source implementation of the Heroes of Might and Magic II game engine. To play the updated version, you need the original Heroes of Might and Magic II or at least its demo version. The latter is available by the script distributed with the source code. Depending on the operating system, you need to choose the appropriate version.
After a successful project build, I decided to get a little nostalgic and run the game. For convenience, I slightly edited the fheroes2.cfg file by setting the parameters:
heroes speed = 10
ai speed = 10
battle speed = 10I also set its resolution in the videomode parameter.
After all manipulations, I started the game and saw a familiar home screen:
If you set wrong screen resolution or don't want to tinker around with the config file, open the game in full-screen mode by pressing f4.
Next, I chose the standard game. Since I downloaded the demo version, the only available map was Broken Alliance.
It is very convenient that the windows with the map, heroes, and settings can be moved to the needed parts of the screen. Some reviews claimed the AI had problems in previous game versions. Now it masters the map quite quickly and fights well. Playing around with it was a real blast.
At the time of writing the last available project version was 0.8.4. It enhanced game performance on low-performance devices, added a large number of gameplay and cosmetic features that you can check out here. The following note got my attention: "fixed more than a hundred bugs compared to the previous version". The authors seem to carefully monitor code quality: as we can see from the project page on GitHub, they regularly use a Sonar Cxx static analyzer, occasionally perform checks by Cppcheck.
It seems to me that if astrologers announce a static analysis week and developers add PVS-Studio to their utilities list, there will be even fewer bugs. Let's make sure of this by looking at a few erroneous code snippets I found using this tool. Just in case, developers of open projects can use the PVS-Studio analyzer for free.
For a change, let's start with shallow code optimizations rather than actual errors. Deep optimizations require profilers, so here we'll be limited to low hanging fruits. Static analyzers often lack information on how particular code operates and therefore are not able to show actual bottlenecks. That's why we use "micro-optimizations" for a set of PVS-Studio warnings about increasing the speed of work.
We do not expect that tips in this article will utterly help speed up the game. I just wanted to pay attention to this set of diagnostics that is usually not covered in our regular articles about checking open projects and therefore remains in the shadow.
Warning N1
V823 Decreased performance. Object may be created in-place in the 'list' container. Consider replacing methods: 'push_back' -> 'emplace_back'. tools.cpp 231
std::list<std::string> StringSplit( const std::string & str, ....)
{
std::list<std::string> list;
size_t pos1 = 0;
size_t pos2 = std::string::npos;
while ( pos1 < str.size()
&& std::string::npos != (pos2 = str.find(sep, pos1)))
{
list.push_back( str.substr( pos1, pos2 - pos1 ) );
pos1 = pos2 + sep.size();
}
....
}The analyzer suggests that in this case it will be more efficient to use the emplace_back method. In general, a simple change from push_back to emplace_back won't yield performance improvement when the argument is an rvalue. However, in our case, the std::string has a constructor accepting two iterators (see constructor #6). It will allow us to avoid a redundant move constructor call when emplace_back is used:
std::list<std::string> StringSplit( const std::string & str, ....)
{
std::list<std::string> list;
size_t pos1 = 0;
size_t pos2 = std::string::npos;
while ( pos1 < str.size()
&& std::string::npos != (pos2 = str.find(sep, pos1)))
{
list.emplace_back(str.begin() + pos1, str.begin() + pos2);
pos1 = pos2 + sep.size();
}
....
}The analyzer found more than 100 of such warnings which provides insight into the importance of the issue. Here are some of them:
Warning N2
V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. tools.cpp 216
void StringReplace( std::string & dst,
const char * pred,
const std::string & src )
{
size_t pos = std::string::npos;
while ( std::string::npos != ( pos = dst.find( pred ) ) )
{
dst.replace( pos, std::strlen( pred ), src );
}
}In this case, the strlen function is called at each loop iteration, and the size of the pred string does not change. The most cliche'd way to make it simpler is to calculate the string length outside the loop and make it constant.
void StringReplace( std::string & dst,
const char * pred,
const std::string & src )
{
size_t pos = std::string::npos;
const size_t predSize = std::strlen( pred);
while ( std::string::npos != ( pos = dst.find( pred ) ) )
{
dst.replace( pos, predSize, src );
}
}Warning N3
V827 Maximum size of the 'optionAreas' vector is known at compile time. Consider pre-allocating it by calling optionAreas.reserve(6) battle_dialogs.cpp 217
void Battle::DialogBattleSettings( .... )
{
std::vector<fheroes2::Rect> optionAreas;
optionAreas.push_back( fheroes2::Rect( pos_rt.x + 36,
pos_rt.y + 47,
panelWidth,
panelHeight ) );
optionAreas.push_back( fheroes2::Rect( pos_rt.x + 128,
pos_rt.y + 47,
panelWidth,
panelHeight ) );
optionAreas.push_back( fheroes2::Rect( pos_rt.x + 220,
pos_rt.y + 47,
panelWidth,
panelHeight ) );
optionAreas.push_back( fheroes2::Rect( pos_rt.x + 36,
pos_rt.y + 157,
panelWidth,
panelHeight ) );
optionAreas.push_back( fheroes2::Rect( pos_rt.x + 128,
pos_rt.y + 157,
panelWidth,
panelHeight ) );
optionAreas.push_back( fheroes2::Rect( pos_rt.x + 220,
pos_rt.y + 157,
panelWidth,
panelHeight ) );
}The analyzer detected std::vector, the maximum size of which is known at compile time. Before filling the container, it would be much more efficient to call:
optionAreas.reserve(6);In this case, push_back calls will not reallocate the internal buffer in the vector and move the elements to a new memory area. Another option is to rewrite this code using std::array.
Warnings N4. 0, 4.1...4.7
The analyzer found some interesting Clear functions, see the code below. What's interesting, such behavior can be found in other code parts.
void Clear( void )
{
if ( armyBar )
delete armyBar;
if ( artifactsBar )
delete artifactsBar;
if ( secskillsBar )
delete secskillsBar;
if ( primskillsBar )
delete primskillsBar;
}
void Clear( void )
{
if ( armyBarGuard )
delete armyBarGuard;
if ( armyBarGuest )
delete armyBarGuest;
if ( dwellingsBar )
delete dwellingsBar;
}In this case, we can refactor the code by removing all checks for null pointers from the functions. The delete operator handles the code correctly anyway. This may not be a performance benefit (the compiler will remove the checks itself), but it will make the code simpler and more readable.
Warning N5
The analyzer has issued 2 warnings for this code fragment:
void Battle::Interface::RedrawActionBloodLustSpell( Unit & target )
{
std::vector<std::vector<uint8_t> > originalPalette;
if ( target.Modes( SP_STONE ) )
{
originalPalette.push_back( PAL::GetPalette( PAL::GRAY ) );
}
else if ( target.Modes( CAP_MIRRORIMAGE ) )
{
originalPalette.push_back( PAL::GetPalette( PAL::MIRROR_IMAGE ) );
}
if ( !originalPalette.empty() )
{
for ( size_t i = 1; i < originalPalette.size(); ++i )
{
originalPalette[0] = PAL::CombinePalettes( originalPalette[0],
originalPalette[i] );
}
fheroes2::ApplyPalette( unitSprite, originalPalette[0] );
}
....
}As we can see, the programmer made a mistake in the algorithm. As the function runs, the originalPalette vector increases in size by one or remains empty. We'll enter the if statement above only when originalPalette.size() equals one. Therefore, the i variable will never be less than the size of the vector. This is how we get a fragment of unreachable code.
Warning N6
V547 Expression 'palette.empty()' is always true. image_tool.cpp 32
const std::vector<uint8_t> PALPAlette()
{
std::vector<uint8_t> palette;
if (palette.empty()) // <=
{
palette.resize( 256 * 3 );
for ( size_t i = 0; i < palette.size(); ++i )
{
palette[i] = kb_pal[i] << 2;
}
}
return palette;
}In this case, the analyzer sees that we unconditionally create an empty vector. So, this check is redundant. We can remove it and make the code simpler.
Warning N7
V668 There is no sense in testing the 'listlog' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. battle_interface.cpp 986
Battle::Interface::Interface(....)
{
....
listlog = new StatusListBox();
....
if ( listlog )
{
....
}
....
}The analyzer detected that the pointer value, returned by the new operator is checked for null. This usually means that a program won't behave in the way the programmer expects in case it's not possible to allocate memory. Since the new operator was unable to allocate memory, according to the C++ standard, we get the std::bad_alloc() exception. This means this check is redundant.
Here are two similar warnings:
Warning N8
V595 The '_currentUnit' pointer was utilized before it was verified against nullptr. Check lines: 2336, 2358. battle_interface.cpp 2336
void Battle::Interface::MouseLeftClickBoardAction( .... )
{
....
themes = GetSwordCursorDirection( Board::GetDirection( index,
_currentUnit->GetHeadIndex()));
....
if ( _currentUnit )
{
....
}
....
}The _currentUnit pointer is first dereferenced and then checked for NULL. This can mean one of two obvious things: undefined behavior will take place if the pointer is null, or the pointer can't be null and the program will always work correctly. If the first option is implied, the check should be performed before dereferencing. In the second case, one can omit the redundant check.
In my opinion, the project is now very close to the original version of the game. As for the code, it is of quite high quality. Not a surprise, because the developers use several static analyzers. However, there are no limits to perfection. If used by project developers, PVS-Studio can help reduce even more bugs. Don't forget it's free for open-source projects.
In conclusion, kudos to the developers - the engine is really cool! If you are looking for a decent interesting open-source project to take part in, fheroes2 is just what you need.
0
0
0
0