Главная
Блог разработчиков phpBB
 
+ 17 предустановленных модов
+ SEO-оптимизация форума
+ авторизация через соц. сети
+ защита от спама

Второстепенный итог: проверяем CryEngine 3 SDK с поддержкой CppCat

Anna | 24.06.2014 | нет комментариев
CryEngine 3 SDK and PVS-Studio
Мы завершили сопоставлять статические анализаторы кода CppCat, Cppcheck, PVS-Studio и анализатор встроенный в Visual Studio 2013. В ходе этого было проверено больше 10 открытых планов. И про некоторые из них дозволено написать статьи. Вот очередная такая статья о итогах проверки плана CryEngine 3 SDK.

CryEngine 3 SDK

Wikipedia: CryEngine 3 SDK — комплект программных инструментов для разработки компьютерных игр на игровом движке CryEngine 3. CryEngine 3 SDK, как и CryEngine 3, разработан и поддерживается немецкой компанией Crytek. CryEngine 3 SDK является проприетарным бесплатным (freeware) для скачивания, установки и применения средством разработки, с поддержкой которого всякий желающий может разрабатывать компьютерные игры и даром распространять их. При торговом распространении (продажах) игр, разработанных на CryEngine 3 SDK, нужно делать лицензионные отчисления в Crytek.

CppCat

Давайте посмотрим, что увлекательного дозволено обнаружить в этой библиотеке с поддержкой анализатораCppCat.

Отчего с поддержкой CppCat, а не с поддержкой PVS-Studio? Легко для многообразия. Хочется, Дабы было видно, что CppCat в обзоре всеобщего назначения примерно не уступает 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. Так что на самом деле оплошность никак себя не проявит.

Подробнее, чем CppCat отличается от PVS-Studio рассказано в статье “Альтернатива PVS-Studio за $250“.

Итоги проверки

Короткий итог — нужно пользоваться статическими анализаторами кода. И тогда ошибок в программе станет поменьше и мне не о чем будет писать статьи.

Двойная проверка

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

Схожие весьма подозрительные места:

  • environmentalweapon.cpp 964
  • persistantstats.cpp 610
  • persistantstats.cpp 714
  • recordingsystem.cpp 8924
  • movementtransitions.cpp 610
  • gamerulescombicaptureobjective.cpp 1692
  • vehiclemovementhelicopter.cpp 588

Неинициализированная ячейка массива

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’ состоит из 2-х элементов. В конструкторе массив хотели инициализировать. Но не получилось.

Не там поставили скобку

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

Редкая и увлекательная оплошность. Не там поставлена закрывающаяся скобка.

Дело в том, что 2-й довод функции 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’.

Есть ещё несколько таких подозрительных циклов:

  • gunturret.cpp 1647
  • vehiclemovementbase.cpp 2362
  • vehiclemovementbase.cpp 2382

Необычные присваивания

Фрагмент 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

И ещё несколько таких аномальных присваиваний :

  • The ‘bExecuteCommandLine’ variable. Check lines: 628, 630. isystem.h 630
  • The ‘flags’ variable. Check lines: 2807, 2808. physinterface.h 2808
  • The ‘entTypes’ Variable. Check lines: 2854, 2856. physinterface.h 2856
  • The ‘geomFlagsAny’ variable. Check lines: 2854, 2857. physinterface.h 2857
  • The ‘m_pLayerEffectParams’ variable. Check lines: 762, 771. ishader.h 771

Необходимо быть опрятным с именами сущностей

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

Оператор new

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’ генерирует исключение.

Другие места, ожидающие рефакторинга:

  • cry_math.h 73
  • datapatchdownloader.cpp 106
  • datapatchdownloader.cpp 338
  • game.cpp 1671
  • game.cpp 4478
  • persistantstats.cpp 1235
  • sceneblurgameeffect.cpp 366
  • killcamgameeffect.cpp 369
  • downloadmgr.cpp 1090
  • downloadmgr.cpp 1467
  • matchmakingtelemetry.cpp 69
  • matchmakingtelemetry.cpp 132
  • matchmakingtelemetry.cpp 109
  • telemetrycollector.cpp 1407
  • telemetrycollector.cpp 1470
  • telemetrycollector.cpp 1467
  • telemetrycollector.cpp 1479
  • statsrecordingmgr.cpp 1134
  • statsrecordingmgr.cpp 1144
  • statsrecordingmgr.cpp 1267
  • statsrecordingmgr.cpp 1261
  • featuretester.cpp 876
  • menurender3dmodelmgr.cpp 1373

Итоги

Особенных итогов у меня нет. Но представляю, сколько каждого увлекательного дозволено обнаружить не в CryEngine 3 SDK, а в самом движке CryEngine 3.

Хочу каждому безбажного кода!

P.S.

Кто-то непрерывно отправляет иноземцам ссылку на русскую статью. Вот перевод: A Spin-off: CryEngine 3 SDK Checked with CppCat.

Результаты на вопросы

P.P.S.

Зачастую к нашим статьям задают одни и те же вопросы. Результаты на них мы собрали тут: Результаты на вопросы читателей статей про PVS-Studio и CppCat, версия 2014.

Источник: programmingmaster.ru

Оставить комментарий
Форум phpBB, русская поддержка форума phpBB
Рейтинг@Mail.ru 2008 - 2017 © BB3x.ru - русская поддержка форума phpBB