Мы закончили сравнивать статические анализаторы кода Cppcheck, PVS-Studio и анализатор встроенный в Visual Studio 2013. В ходе этого было проверено более 10 открытых проектов. И про некоторые из них можно написать статьи. Вот очередная такая статья о результатах проверки проекта CryEngine 3 SDK.
Wikipedia: CryEngine 3 SDK — набор программных инструментов для разработки компьютерных игр на игровом движке CryEngine 3. CryEngine 3 SDK, как и CryEngine 3, разработан и поддерживается немецкой компанией Crytek. CryEngine 3 SDK является проприетарным бесплатным (freeware) для скачивания, установки и использования средством разработки, с помощью которого любой желающий может разрабатывать компьютерные игры и бесплатно распространять их. При коммерческом распространении (продажах) игр, разработанных на CryEngine 3 SDK, необходимо делать лицензионные отчисления в Crytek.
Давайте посмотрим, что интересного можно найти в этой библиотеке с помощью анализатора PVS-Studio.
PVS-Studio находит немного больше, если включить 3 уровень предупреждений. Пример:
static void GetNameForFile(
const char* baseFileName,
const uint32 fileIdx,
char outputName[512] )
{
assert(baseFileName != NULL);
sprintf( outputName, "%s_%d", baseFileName, fileIdx );
}V576 Incorrect format. Consider checking the fourth actual argument of the 'sprintf' function. The SIGNED integer type argument is expected. igame.h 66
Формально, для распечатки беззнаковой переменной 'fileIdx' надо использовать "%u". Однако сомнительно, что эта переменная достигнет значения больше, чем INT_MAX. Так что на самом деле ошибка никак себя не проявит.
Краткий результат - надо пользоваться статическими анализаторами кода. И тогда ошибок в программе станет меньше и мне не о чем будет писать статьи.
void CVehicleMovementArcadeWheeled::InternalPhysicsTick(float dt)
{
....
if (fabsf(m_movementAction.rotateYaw)>0.05f ||
vel.GetLengthSquared()>0.001f ||
m_chassis.vel.GetLengthSquared()>0.001f ||
angVel.GetLengthSquared()>0.001f ||
angVel.GetLengthSquared()>0.001f)
....
}V501 There are identical sub-expressions 'angVel.GetLengthSquared() > 0.001f' to the left and to the right of the '||' operator. vehiclemovementarcadewheeled.cpp 3300
Два раза выполняется проверка "angVel.GetLengthSquared()>0.001f". Одна проверка лишняя. Или это опечатка, за-за которой не проверяется другое значение.
Фрагмент N1.
void CVicinityDependentObjectMover::HandleEvent(....)
{
....
else if ( strcmp(szEventName, "ForceToTargetPos") == 0 )
{
SetState(eObjectRangeMoverState_MovingTo);
SetState(eObjectRangeMoverState_Moved);
ActivateOutputPortBool( "OnForceToTargetPos" );
}
else if ( strcmp(szEventName, "ForceToTargetPos") == 0 )
{
SetState(eObjectRangeMoverState_MovingTo);
SetState(eObjectRangeMoverState_Moved);
ActivateOutputPortBool( "OnForceToTargetPos" );
}
....
}V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 255, 261. vicinitydependentobjectmover.cpp 255
Мне кажется, что этот код написан с помощью Copy-Paste. И ещё мне кажется, что в этом коде после копирования забыли что-то исправить.
Фрагмент N2. Очень странным образом реализована функция ShouldGiveLocalPlayerHitableFeedbackOnCrosshairHoverForEntityClass(). Вот это я понимаю ИМЯ!
bool CGameRules::
ShouldGiveLocalPlayerHitableFeedbackOnCrosshairHoverForEntityClass
(const IEntityClass* pEntityClass) const
{
assert(pEntityClass != NULL);
if(gEnv->bMultiplayer)
{
return
(pEntityClass == s_pSmartMineClass) ||
(pEntityClass == s_pTurretClass) ||
(pEntityClass == s_pC4Explosive);
}
else
{
return
(pEntityClass == s_pSmartMineClass) ||
(pEntityClass == s_pTurretClass) ||
(pEntityClass == s_pC4Explosive);
}
}V523 The 'then' statement is equivalent to the 'else' statement. gamerules.cpp 5401
Аналогичные крайне подозрительные места:
TDestructionEventId destructionEvents[2];
SDestructibleBodyPart()
: hashId(0)
, healthRatio(0.0f)
, minHealthToDestroyOnDeathRatio(0.0f)
{
destructionEvents[0] = -1;
destructionEvents[0] = -1;
}V519 The 'destructionEvents[0]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 75, 76. bodydestruction.h 76
Массив 'destructionEvents' состоит из двух элементов. В конструкторе массив хотели инициализировать. Но не получилось.
bool ShouldRecordEvent(size_t eventID, IActor* pActor=NULL) const;
void CActorTelemetry::SubscribeToWeapon(EntityId weaponId)
{
....
else if(pMgr->ShouldRecordEvent(eSE_Weapon), pOwnerRaw)
....
}V639 Consider inspecting the expression for 'ShouldRecordEvent' function call. It is possible that one of the closing ')' brackets was positioned incorrectly. actortelemetry.cpp 288
Редкая и интересная ошибка. Не там поставлена закрывающаяся скобка.
Дело в том, что второй аргумент функции ShouldRecordEvent() является необязательным. Получается, что в начале вызывается функция ShouldRecordEvent(). Затем, оператор запятая ',' возвращает значение, стоящее с права. Условие зависит только от переменной 'pOwnerRaw'.
В общем, чёрте что получилось.
virtual void ProcessEvent(....)
{
....
string pMessage = ("%s:", currentSeat->GetSeatName());
....
}V521 Such expressions using the ',' operator are dangerous. Make sure the expression '"%s:", currentSeat->GetSeatName()' is correct. flowvehiclenodes.cpp 662
В этом выражении переменной pMessage присваивается значение currentSeat->GetSeatName(). Никакого форматирования не происходит. Как результат в строке будет отсутствовать двоеточие ':'. Хоть и мелочь, но ошибка.
Должно было быть:
string pMessage =
string().Format("%s:", currentSeat->GetSeatName());Фрагмент N1.
inline bool operator != (const SEfResTexture &m) const
{
if (stricmp(m_Name.c_str(), m_Name.c_str()) != 0 ||
m_TexFlags != m.m_TexFlags ||
m_bUTile != m.m_bUTile ||
m_bVTile != m.m_bVTile ||
m_Filter != m.m_Filter ||
m_Ext != m.m_Ext ||
m_Sampler != m.m_Sampler)
return true;
return false;
}V549 The first argument of 'stricmp' function is equal to the second argument. ishader.h 2089
Если не заметили ошибку, то подсказываю. Строка m_Name.c_str() сравнивается сама с собой. Должно быть написано:
stricmp(m_Name.c_str(), m.m_Name.c_str())Фрагмент N2. Теперь логическая ошибка:
SearchSpotStatus GetStatus() const { return m_status; }
SearchSpot* SearchGroup::FindBestSearchSpot(....)
{
....
if(searchSpot.GetStatus() != Unreachable ||
searchSpot.GetStatus() != BeingSearchedRightAboutNow)
....
}V547 Expression is always true. Probably the '&&' operator should be used here. searchmodule.cpp 469
Проверка в этом коде не имеет смысла. Приведу аналогию:
if (A != 1 || A != 2)Условие всегда выполняется.
Фрагмент N3.
const CCircularBufferTimeline *
CCircularBufferStatsContainer::GetTimeline(
size_t inTimelineId) const
{
....
if (inTimelineId >= 0 && (int)inTimelineId < m_numTimelines)
{
tl = &m_timelines[inTimelineId];
}
else
{
CryWarning(VALIDATOR_MODULE_GAME,VALIDATOR_ERROR,
"Statistics event %" PRISIZE_T
" is larger than the max registered of %"
PRISIZE_T ", event ignored",
inTimelineId,m_numTimelines);
}
....
}V547 Expression 'inTimelineId >= 0' is always true. Unsigned type value is always >= 0. circularstatsstorage.cpp 31
Фрагмент N4.
inline typename CryStringT<T>::size_type
CryStringT<T>::rfind( value_type ch,size_type pos ) const
{
const_str str;
if (pos == npos) {
....
} else {
if (pos == npos)
pos = length();
....
}V571 Recurring check. The 'if (pos == npos)' condition was already verified in line 1447. crystring.h 1453
Присваивание "pos = length()" никогда не будет выполнено.
Аналогичный код здесь: cryfixedstring.h 1297
Программисты очень любят проверять указатель на равенство нулю. Вот если бы они ещё знали, как часто они делают это неправильно. Проверяют, когда уже поздно.
Приведу один пример, остальное перечислю списком.
IScriptTable *p;
bool Create( IScriptSystem *pSS, bool bCreateEmpty=false )
{
if (p) p->Release();
p = pSS->CreateTable(bCreateEmpty);
p->AddRef();
return (p)?true:false;
}V595 The 'p' pointer was utilized before it was verified against nullptr. Check lines: 325, 326. scripthelpers.h 325
Обещанный список из 35 сообщений: CryEngineSDK-595.txt
void AddSample( T x )
{
m_index = ++m_index % N;
....
}V567 Undefined behavior. The 'm_index' variable is modified while being used twice between sequence points. inetwork.h 2303
void CWeapon::AccessoriesChanged(bool initialLoadoutSetup)
{
....
for (int i = 0; i < numZoommodes; i++)
{
CIronSight* pZoomMode = ....
const SZoomModeParams* pCurrentParams = ....
const SZoomModeParams* pNewParams = ....
if(pNewParams != pCurrentParams)
{
pZoomMode->ResetSharedParams(pNewParams);
}
break;
}
....
}V612 An unconditional 'break' within a loop. weapon.cpp 2854
Тело цикла будет выполнять только один раз. Причина - безусловный оператор 'break'. При этом, в цикле нет операторов 'continue'.
Есть ещё несколько таких подозрительных циклов:
Фрагмент N1.
void CPlayerStateGround::OnPrePhysicsUpdate(....)
{
....
modifiedSlopeNormal.z = modifiedSlopeNormal.z;
....
}V570 The 'modifiedSlopeNormal.z' variable is assigned to itself. playerstateground.cpp 227
Фрагмент N2.
const SRWIParams& Init(....)
{
....
objtypes=ent_all;
flags=rwi_stop_at_pierceable;
org=_org;
dir=_dir;
objtypes=_objtypes;
....
}V519 The 'objtypes' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2807, 2808. physinterface.h 2808
Члену класса 'objtypes' два раза присваивается значение.
Фрагмент N3.
void SPickAndThrowParams::SThrowParams::SetDefaultValues()
{
....
maxChargedThrowSpeed = 20.0f;
maxChargedThrowSpeed = 15.0f;
}V519 The 'maxChargedThrowSpeed' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1284, 1285. weaponsharedparams.cpp 1285
И ещё несколько таких аномальных присваиваний:
void CGamePhysicsSettings::Debug(....) const
{
....
sprintf_s(buf, bufLen, pEntity->GetName());
....
}V618 It's dangerous to call the 'sprintf_s' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); gamephysicssettings.cpp 174
Пожалуй, это не ошибка, но опасный код. Если вдруг в имени сущности встретится символ '%', то результаты могут быть весьма неожиданными.
CPersistantStats::SEnemyTeamMemberInfo
*CPersistantStats::GetEnemyTeamMemberInfo(EntityId inEntityId)
{
....
insertResult.first->second.m_entityId;
....
}V607 Ownerless expression 'insertResult.first->second.m_entityId'. persistantstats.cpp 4814
Одинокое выражение, которое ничего не делает. Ошибка? Недописанный код?
Ещё одно место в коде: recordingsystem.cpp 2671
bool CreateWriteBuffer(uint32 bufferSize)
{
FreeWriteBuffer();
m_pWriteBuffer = new uint8[bufferSize];
if (m_pWriteBuffer)
{
m_bufferSize = bufferSize;
m_bufferPos = 0;
m_allocated = true;
return true;
}
return false;
}V668 There is no sense in testing the 'm_pWriteBuffer' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. crylobbypacket.h 88
Код устарел. Теперь при ошибки выделения памяти оператор 'new' генерирует исключение.
Другие места, ждущие рефакторинга:
Особенных выводов у меня нет. Зато представляю, сколько всего интересного можно найти не в CryEngine 3 SDK, а в самом движке CryEngine 3.
Желаю всем безбажного кода!