Серьёзные ошибки в коде CryEngine V

2017-04-04 в 6:54, admin, рубрики: c++, CryEngine, crytek, game development, open source, pvs-studio, Блог компании PVS-Studio, обзор кода, Программирование, разработка игр

В мае 2016 года немецкая компания Crytek решила опубликовать на GitHub исходный код игрового движка CryEngine V. Проект находится в стадии активной разработки, что влечёт за собой появление множества ошибок в коде. Мы уже проверяли проект с помощью PVS-Studio для Windows, а теперь смогли проверить проект с помощью PVS-Studio для Linux. Материала снова набралось на большую статью с описанием только очень серьёзных ошибок.

Введение

CryEngine — игровой движок, созданный немецкой компанией Crytek в 2002 году, и первоначально используемый в шутере от первого лица Far Cry. На CryEngine разных версий сделано много отличных игр от нескольких игровых студий, которые лицензировали движок: Far Cry, Crysis, Entropia Universe, Blue Mars, Warface, Homefront: The Revolution, Sniper: Ghost Warrior, Armored Warfare, Evolve и многие другие. В марте 2016 года компания Crytek анонсировала выход своего нового движка CryEngine V и вскоре опубликовала исходный код на GitHub.

Для проверки открытого исходного кода использовался статический анализатор PVS-Studio версии 6.14 для Linux. Теперь разработчикам кроссплатформенных проектов стало ещё удобнее следить за качеством кода с помощью одного инструмента анализа кода. Скачать версию для Linux можно в виде архива или пакета для пакетного менеджера. Для большинства дистрибутивов можно настроить установку и обновление, используя наш репозиторий.

В статью вошли предупреждения общего назначения и только уровня достоверности «High» (есть ещё Medium и Low). Честно говоря, я не осилил досмотреть внимательно и все «High» предупреждения, т.к. почти сразу насобирал ошибок для статьи при быстром просмотре. За проект я брался несколько раз и долго не мог найти время написать обзорную статью, поэтому про приведённые баги могу сказать, что они живут в коде уже не один месяц. Также некоторые ошибки не исправили из предыдущей статьи о проверке проекта.

Скачать и проверить исходный код в Linux было очень просто. Вот список всех необходимых команд:

mkdir ~/projects && cd ~/projects
git clone https://github.com/CRYTEK/CRYENGINE.git
cd CRYENGINE/
git checkout main
chmod +x ./download_sdks.py
./download_sdks.py
pvs-studio-analyzer trace -- 
  sh ./cry_waf.sh build_linux_x64_clang_profile -p gamesdk
pvs-studio-analyzer analyze 
  -l /path/to/PVS-Studio.lic 
  -o ~/projects/CRYENGINE/cryengine.log 
  -r ~/projects/CRYENGINE/ 
  -C clang++-3.8 -C clang-3.8 
  -e ~/projects/CRYENGINE/Code/SDKs 
  -j4
plog-converter -a GA:1,2 -t tasklist 
  -o ~/projects/CRYENGINE/cryengine_ga.tasks 
  ~/projects/CRYENGINE/cryengine.log

Файл отчёта cryengine_ga.tasks можно открыть и просмотреть в QtCreator. Что же удалось найти в исходном коде CryEngine V?

Несчастная функция Active()

V501 There are identical sub-expressions to the left and to the right of the '==' operator: bActive == bActive LightEntity.h 124

void SetActive(bool bActive)
{
  if (bActive == bActive)
    return;

  m_bActive = bActive;
  OnResetState();
}

Из-за опечатки функция ничего не делает. Мне кажется, если бы был конкурс «Мисс Опечатка», то этот фрагмент кода точно бы занял первое место. Думаю, эта ошибка имеет все шансы попасть в раздел "C/C++ bugs of the month".

Но это ещё не всё, вот функция из другого класса:

V501 There are identical sub-expressions 'm_staticObjects' to the left and to the right of the '||' operator. FeatureCollision.h 66

class CFeatureCollision : public CParticleFeature
{
public:
  CRY_PFX2_DECLARE_FEATURE

public:
  CFeatureCollision();
  ....

  bool  IsActive() const  { return m_terrain ||
                                   m_staticObjects ||
                                   m_staticObjects; }
  ....
  bool m_terrain;
  bool m_staticObjects;
  bool m_dynamicObjects;
};

Тут в функции IsActive() два раза используется переменная m_staticObjects, хотя рядом есть неиспользуемая переменная m_dynamicObjects. Возможно, в коде хотели использовать именно её.

Code above has no bugs

V547 Expression 'outArrIndices[i] < 0' is always false. Unsigned type value is never < 0. CGFLoader.cpp 881

static bool CompactBoneVertices(....,
  DynArray<uint16>& outArrIndices, ....)           // <= uint16
{
  ....
  outArrIndices.resize(3 * inFaceCount, -1);

  int outVertexCount = 0;
  for (int i = 0; i < verts.size(); ++i)
  {
    ....
    outArrIndices[....] = outVertexCount - 1;
  }

  // Making sure that the code above has no bugs   // <= LOL
  for (int i = 0; i < outArrIndices.size(); ++i)
  {
    if (outArrIndices[i] < 0)                      // <= LOL
    {
      return false;
    }
  }
  
  return true;
}

Эта ошибка достойна отдельного раздела. Вообще, в коде CryEngine ну очень много мест, где беззнаковые переменные бессмысленно сравнивают с нулём. Таких мест сотни, но этому фрагменту хочется уделить особое внимание, т.к. код писался намерено.

Так вот, есть массив беззнаковых чисел — outArrIndices. Далее массив заполняется по некому алгоритму. После чего выполняется гениальная проверка каждого элемента массива, чтобы среди них не было ни одного отрицательного числа. Элементы массива имеют тип uint16.

Ошибки при работе с памятью

V512 A call of the 'memcpy' function will lead to underflow of the buffer 'hashableData'. GeomCacheRenderNode.cpp 285

void CGeomCacheRenderNode::Render(....)
{
  ....
  CREGeomCache* pCREGeomCache = iter->second.m_pRenderElement;
  ....
  uint8 hashableData[] =
  {
    0, 0, 0, 0, 0, 0, 0, 0,
    (uint8)std::distance(pCREGeomCache->....->begin(), &meshData),
    (uint8)std::distance(meshData....->....begin(), &chunk),
    (uint8)std::distance(meshData.m_instances.begin(), &instance)
  };

  memcpy(hashableData, pCREGeomCache, sizeof(pCREGeomCache));
  ....
}

Обратите внимание на аргументы функции memcpy(). Объект pCREGeomCache планируют скопировать в массив hashableData, но случайно вычислили с помощью оператора sizeof не размер объекта, а размер указателя. Из-за ошибки объект копируется не полностью, а всего 4 или 8 байт.

V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'this' class object. ClipVolumeManager.cpp 145

void
CClipVolumeManager::GetMemoryUsage(class ICrySizer* pSizer) const
{
  pSizer->AddObject(this, sizeof(this));
  for (size_t i = 0; i < m_ClipVolumes.size(); ++i)
    pSizer->AddObject(m_ClipVolumes[i].m_pVolume);
}

Похожую ошибку допустили, вычислив вместо размера класса, размер указателя this. Правильный вариант: sizeof(*this).

V530 The return value of function 'release' is required to be utilized. ClipVolumes.cpp 492

vector<unique_ptr<CFullscreenPass>> m_jitteredDepthPassArray;

void CClipVolumesStage::PrepareVolumetricFog()
{
  ....
  for (int32 i = 0; i < m_jitteredDepthPassArray.size(); ++i)
  {
    m_jitteredDepthPassArray[i].release();
  }

  m_jitteredDepthPassArray.resize(depth);

  for (int32 i = 0; i < depth; ++i)
  {
    m_jitteredDepthPassArray[i] = CryMakeUnique<....>();
    m_jitteredDepthPassArray[i]->SetViewport(viewport);
    m_jitteredDepthPassArray[i]->SetFlags(....);
  }
  ....
}

Если обратиться к документации по классу std::unique_ptr, то функция release() должна использоваться примерно следующим образом:

std::unique_ptr<Foo> up(new Foo());
Foo* fp = up.release();
delete fp;

Скорее всего, в данном фрагменте кода хотели использовать функцию reset() вместо release().

V549 The first argument of 'memcpy' function is equal to the second argument. ObjectsTree_Serialize.cpp 1135

void COctreeNode::LoadSingleObject(....)
{
  ....
  float* pAuxDataDst = pObj->GetAuxSerializationDataPtr(....);
  const float* pAuxDataSrc = StepData<float>(....);
  memcpy(pAuxDataDst, pAuxDataDst, min(....) * sizeof(float));
  ....
}

Забыли передать pAuxDataSrc в функцию memcpy(). Вместо этого в качестве источника и приёмника используют одну и ту же переменную pAuxDataDst. Никто не застрахован от опечаток. Кстати, жалеющие могут проверить свою внимательность, пройдя наш тест по выявлению подобных ошибок: q.viva64.com.

Странный код

V501 There are identical sub-expressions to the left and to the right of the '||' operator: val == 0 || val == — 0 XMLCPB_AttrWriter.cpp 363

void CAttrWriter::PackFloatInSemiConstType(float val, ....)
{
  uint32 type = PFSC_VAL;

  if (val == 0 || val == -0)  // <=
    type = PFSC_0;
  else if (val == 1)
    type = PFSC_1;
  else if (val == -1)
    type = PFSC_N1;

  ....
}

Разработчики планировали сравнить вещественную переменную val с нулём и отрицательным нулём (signed zero / negative zero), но сделали это неправильно. Объявив целочисленные константы, значения нулей стали одинаковыми.

Скорее всего, код стоит исправить следующим образом, объявив константы вещественного типа:

if (val == 0.0f || val == -0.0f)
    type = PFSC_0;

С другой стороны, условное выражение является избыточным, т.к. переменную достаточно сравнивать с обычным нулём. По этой причине оригинальный код выполняется так, как ожидает программист.

Но если необходимо идентифицировать именно отрицательный ноль, то правильно это сделать с помощью функции std::signbit.

V501 There are identical sub-expressions 'm_joints[i].limits[1][j]' to the left and to the right of the '-' operator. articulatedentity.cpp 1326

int CArticulatedEntity::Step(float time_interval)
{
  ....
  for (j=0;j<3;j++) if (!(m_joints[i].flags & angle0_locked<<j)&&
    isneg(m_joints[i].limits[0][j]-m_joints[i].qext[j]) +
    isneg(m_joints[i].qext[j]-m_joints[i].limits[1][j]) + 
    isneg(m_joints[i].limits[1][j]-m_joints[i].limits[1][j]) < 2)
  {
    ....
}

В последней части условного выражения присутствует вычитание из переменной m_joints[i].limits[1][j] самой себя. Код выглядит подозрительным. В выражении много индексов, возможно, в одном из них ошибка.

Ещё одно такое место:

V501 There are identical sub-expressions 'm_joints[op[1]].limits[1][i]' to the left and to the right of the '-' operator. articulatedentity.cpp 513

V590 Consider inspecting this expression. The expression is excessive or contains a misprint. GoalOp_Crysis2.cpp 3779

void COPCrysis2FlightFireWeapons::ParseParam(....)
{
  ....
  bool paused;
  value.GetValue(paused);

  if (paused && (m_State != eFP_PAUSED) &&
                (m_State != eFP_PAUSED_OVERRIDE))
  {
    m_NextState = m_State;
    m_State = eFP_PAUSED;
    m_PausedTime = 0.0f;
    m_PauseOverrideTime = 0.0f;
  }
  else if (!paused && (m_State == eFP_PAUSED) &&        // <=
                      (m_State != eFP_PAUSED_OVERRIDE)) // <=
  {
    m_State = m_NextState;
    m_NextState = eFP_STOP;

    m_PausedTime = 0.0f;
    m_PauseOverrideTime = 0.0f;
  }
  ....
}

Условное выражение написано таким образом, что результат не зависит от подвыражения m_State != eFP_PAUSED_OVERRIDE. Хотя кому я рассказываю, этот фрагмент кода разработчики так и не исправили после первой статьи.

Если кому интересно, то подобную разновидность ошибок я уже описывал ранее в отдельной статье "Логические выражения в C/C++. Как ошибаются профессионалы".

V529 Odd semicolon ';' after 'for' operator. boolean3d.cpp 1077

int CTriMesh::Slice(...)
{
  ....
  pmd->pMesh[0]=pmd->pMesh[1] = this;  AddRef();AddRef();
  for(pmd0=m_pMeshUpdate; pmd0->next; pmd0=pmd0->next); // <=
    pmd0->next = pmd;
  ....
}

Ещё один фрагмент кода, неисправленный с момента предыдущей проверки. По коду по-прежнему не ясно: тут ошибочное форматирование или логическая ошибка?

Про указатели

V522 Dereferencing of the null pointer 'pCEntity' might take place. BreakableManager.cpp 2396

int CBreakableManager::HandlePhysics_UpdateMeshEvent(....)
{
  CEntity* pCEntity = 0;
  ....
  if (pmu && pSrcStatObj && GetSurfaceType(pSrcStatObj))
  {
    ....
    if (pEffect)
    {
      ....
      if (normal.len2() > 0)
        pEffect->Spawn(true, pCEntity->GetSlotWorldTM(...); // <=
    }
  }

  ....

  if (iForeignData == PHYS_FOREIGN_ID_ENTITY)
  {
    pCEntity = (CEntity*)pForeignData;
    if (!pCEntity || !pCEntity->GetPhysicalProxy())
      return 1;
  }
  ....
}

Анализатор обнаружил разыменование нулевого указателя. Код функции написан или отрефакторен таким образом, что образовалась ветка кода, в которой будет использован указатель pCEntity, инициализированный нулём.

Теперь рассмотрим вариант потенциального разыменования нулевого указателя.

V595 The 'pTrack' pointer was utilized before it was verified against nullptr. Check lines: 60, 61. AudioNode.cpp 60

void CAudioNode::Animate(SAnimContext& animContext)
{
  ....
  const bool bMuted = gEnv->IsEditor() && (pTrack->GetFlags() &
    IAnimTrack::eAnimTrackFlags_Muted);
  if (!pTrack || pTrack->GetNumKeys() == 0 ||
       pTrack->GetFlags() & IAnimTrack::eAnimTrackFlags_Disabled)
  {
    continue;
  }
  ....
}

Автор этого фрагмента кода сначала воспользовался указателем pTrack, а на следующей же строке кода проверяет его валидность перед разыменованием. Скорее всего, это потенциальное место некорректной работы программы.

Предупреждений V595 достаточно много, и в статью они не поместятся. Очень часто такой код является настоящей ошибкой, но благодаря везению, работает корректно.

V571 Recurring check. The 'if (rLightInfo.m_pDynTexture)' condition was already verified in line 69. ObjMan.cpp 70

// Safe memory helpers
#define SAFE_RELEASE(p){ if (p) { (p)->Release(); (p) = NULL; } }

void CObjManager::UnloadVegetationModels(bool bDeleteAll)
{
  ....
  SVegetationSpriteLightInfo& rLightInfo = ....;
  if (rLightInfo.m_pDynTexture)
    SAFE_RELEASE(rLightInfo.m_pDynTexture);
  ....
}

В этом фрагменте кода нет серьёзной ошибки, но не стоит писать лишний код, если в специальный макрос уже включены соответствующие проверки.

Ещё одно место с лишним кодом:

V571 Recurring check. The 'if (m_pSectorGroups)' condition was already verified in line 48. PartitionGrid.cpp 50

V575 The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null. SystemInit.cpp 4045

class CLvlRes_finalstep : public CLvlRes_base
{
  ....
  for (;; )
  {
    if (*p == '/' || *p == '\' || *p == 0)
    {
      char cOldChar = *p;
      *p = 0; // create zero termination
      _finddata_t fd;

      bool bOk = FindFile(szFilePath, szFile, fd);

      if (bOk)
        assert(strlen(szFile) == strlen(fd.name));

      *p = cOldChar; // get back the old separator

      if (!bOk)
        return;

      memcpy((void*)szFile, fd.name, strlen(fd.name)); // <=

      if (*p == 0)
        break;

      ++p;
      szFile = p;
    }
    else ++p;
  }
  ....
}

Возможно, в этом коде допущена ошибка. При копировании строки теряется последний терминальный ноль. В данном случае, необходимо копировать strlen() + 1 символ, либо использовать специальные функции для копирования строк: strcpy или strcpy_s.

Проблемы с запятой

V521 Such expressions using the ',' operator are dangerous. Make sure the expression '!sWords[iWord].empty(), iWord ++' is correct. TacticalPointSystem.cpp 3243

bool CTacticalPointSystem::Parse(....) const
{
  string sInput(sSpec);
  const int MAXWORDS = 8;
  string sWords[MAXWORDS];

  int iC = 0, iWord = 0;
  for (; iWord < MAXWORDS; !sWords[iWord].empty(), iWord++) // <=
  {
    sWords[iWord] = sInput.Tokenize("_", iC);
  }
  ....
}

Обратите внимание на секцию цикла for с счётчиками. Что там делает логическое выражение? Скорее всего, его стоит перенести в другое место и написать так:

for (; iWord < MAXWORDS && !sWords[iWord].empty(); iWord++) {...}

V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. HommingSwarmProjectile.cpp 187

void CHommingSwarmProjectile::HandleEvent(....)
{
  ....
  explodeDesc.normal = -pCollision->n,pCollision->vloc[0];
  ....
}

Ещё один странный фрагмент кода с оператором ','.

Подозрительные условия

V571 Recurring check. The 'if (pos == npos)' condition was already verified in line 1530. CryString.h 1539

//! Find last single character.
// return -1 if not found, distance from beginning otherwise.
template<class T>
inline typename CryStringT<T>::....::rfind(....) const
{
  const_str str;
  if (pos == npos)
  {
    // find last single character
    str = _strrchr(m_str, ch);
    // return -1 if not found, distance from beginning otherwise
    return (str == NULL) ?
      (size_type) - 1 : (size_type)(str - m_str);
  }
  else
  {
    if (pos == npos)
    {
      pos = length();
    }
    if (pos > length())
    {
      return npos;
    }

    value_type tmp = m_str[pos + 1];
    m_str[pos + 1] = 0;
    str = _strrchr(m_str, ch);
    m_str[pos + 1] = tmp;
  }
  return (str == NULL) ?
   (size_type) - 1 : (size_type)(str - m_str);
}

Анализатор обнаружил повторную проверку переменной pos. Из-за такой ошибки часть кода никогда не выполняется. Также в функции есть дублирующийся код, поэтому функцию стоит переписать получше.

Такой код благополучно продублировали ещё в одном месте:

V571 Recurring check. The 'if (pos == npos)' condition was already verified in line 1262. CryFixedString.h 1271

V523 The 'then' statement is equivalent to the 'else' statement. ScriptTable.cpp 789

bool CScriptTable::AddFunction(const SUserFunctionDesc& fd)
{
  ....
  char sFuncSignature[256];
  if (fd.sGlobalName[0] != 0)
    cry_sprintf(sFuncSignature, "%s.%s(%s)", fd.sGlobalName,
      fd.sFunctionName, fd.sFunctionParams);
  else
    cry_sprintf(sFuncSignature, "%s.%s(%s)", fd.sGlobalName,
      fd.sFunctionName, fd.sFunctionParams);
  ....
}

Независимо от содержимого строки, её всё равно пытаются распечатать одинаковым образом. Такого кода тоже очень много в проекте, вот некоторые предупреждения:

V523 The 'then' statement is equivalent to the 'else' statement. BudgetingSystem.cpp 718
V523 The 'then' statement is equivalent to the 'else' statement. D3DShadows.cpp 627
V523 The 'then' statement is equivalent to the 'else' statement. livingentity.cpp 967

Неопределённое поведение

V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. physicalplaceholder.h 25

class CPhysicalEntity;
const int NO_GRID_REG = -1<<14;
const int GRID_REG_PENDING = NO_GRID_REG+1;
const int GRID_REG_LAST = NO_GRID_REG+2;

Анализатор умеет находить несколько типов ошибок, приводящих к неопределённому поведению. Согласно современному стандарту языка, сдвиг отрицательного числа влево приводит к неопределённому поведению.

Вот ещё несколько сомнительных мест:

V610 Undefined behavior. Check the shift operator '<<'. The left operand '~(TFragSeqStorage(0))' is negative. UDPDatagramSocket.cpp 757
V610 Undefined behavior. Check the shift operator '<<'. The right operand ('cpu' = [0..1023]) is greater than or equal to the length in bits of the promoted left operand. CryThreadUtil_posix.h 115
V610 Undefined behavior. Check the shift operator '>>'. The right operand is negative ('comp' = [-1..3]). ShaderComponents.cpp 399
V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. trimesh.cpp 4126
V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. trimesh.cpp 4559
V610 Unspecified behavior. Check the shift operator '>>'. The left operand '-NRAYS' is negative. trimesh.cpp 4618
V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. tetrlattice.cpp 324
V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. tetrlattice.cpp 350
V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. tetrlattice.cpp 617
V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. tetrlattice.cpp 622

Другой тип неопределённого поведения связан с неоднократным изменением переменной между двумя точками следования:

V567 Undefined behavior. The 'm_current' variable is modified while being used twice between sequence points. OperatorQueue.cpp 101

boolCOperatorQueue::Prepare(....)
{
  ++m_current &= 1;
  m_ops[m_current].clear();
  return true;
}

К сожалению, такое место не единственное в коде:

V567 Undefined behavior. The 'm_commandBufferIndex' variable is modified while being used twice between sequence points. XConsole.cpp 180
V567 Undefined behavior. The 'itail' variable is modified while being used twice between sequence points. trimesh.cpp 3119
V567 Undefined behavior. The 'ihead' variable is modified while being used twice between sequence points. trimesh.cpp 3126
V567 Undefined behavior. The 'ivtx' variable is modified while being used twice between sequence points. boolean3d.cpp 957
V567 Undefined behavior. The 'ivtx' variable is modified while being used twice between sequence points. boolean3d.cpp 965
V567 Undefined behavior. The 'ivtx' variable is modified while being used twice between sequence points. boolean3d.cpp 983
V567 Undefined behavior. The 'm_iNextAnimIndex' variable is modified while being used twice between sequence points. HitDeathReactionsDefs.cpp 192

Вопросы к разработчикам

В коде CryEngine V я нашёл интересный способ общения разработчиков через комментарии в коде.

Вот самый забавный комментарий, который я нашёл с помощью предупреждения:

V763 Parameter 'enable' is always rewritten in function body before being used.

void CNetContext::EnableBackgroundPassthrough(bool enable)
{
  SCOPED_GLOBAL_LOCK;
  // THIS IS A TEMPORARY HACK TO MAKE THE GAME PLAY NICELY,
  // ASK peter@crytek WHY IT'S STILL HERE
  enable = false;
  ....
}

Далее я решил с помощью поиска по содержимому файлов поикать подобные тексты и выписал несколько мест:

....
// please ask me when you want to change [tetsuji]
....
// please ask me when you want to change [dejan]
....
//if there are problems with this function, ask Ivo
uint32 numAnims = 
  pCharacter->GetISkeletonAnim()->GetNumAnimsInFIFO(layer);
if (numAnims)
  return pH->EndFunction(true);
....
//ask Ivo for details
//if (pCharacter->GetCurAnimation() &&
//    pCharacter->GetCurAnimation()[0] != '')
//  return pH->EndFunction(pCharacter->GetCurAnimation());
....
/////////////////////////////////////////////////////////////////
// Strange, !do not remove... ask Timur for the meaning of this.
/////////////////////////////////////////////////////////////////
if (m_nStrangeRatio > 32767)
{
  gEnv->pScriptSystem->SetGCFrequency(-1); // lets get nasty.
}
/////////////////////////////////////////////////////////////////
// Strange, !do not remove... ask Timur for the meaning of this.
/////////////////////////////////////////////////////////////////
if (m_nStrangeRatio > 1000)
{
  if (m_pProcess && (m_pProcess->GetFlags() & PROC_3DENGINE))
    m_nStrangeRatio += cry_random(1, 11);
}
/////////////////////////////////////////////////////////////////
....
// tank specific:
// avoid steering input around 0.5 (ask Anton)
....
CryWarning(VALIDATOR_MODULE_EDITOR, VALIDATOR_WARNING,
  "....: Wrong edited item. Ask AlexL to fix this.");
....
// If this renders black ask McJohn what's wrong.
glGenerateMipmap(GL_TEXTURE_2D);
....

Ну и самый главный вопрос к разработчикам: почему они не пользуются специализированными инструментами для повышения качества кода? Я, конечно, имею в виду PVS-Studio. :)

Еще раз хочу отметить, что в статье приведена только часть ошибок. Я не досмотрел до конца даже предупреждения уровня High. Так что проект ещё ждет тех, кто внимательно проверит его. Я, к сожалению, не могу найти столько времени, меня ждут десятки других проектов.

Заключение

За много лет разработки статического анализатора кода я пришёл к выводу, что в командах разработчиков с некоторого количества людей уже невозможно писать код без ошибок. Нет, я не против Code Review, но посчитайте сами, сколько времени понадобится руководителю, чтобы сделать обзор кода 10 человек? А на следующий день? А большего числа людей? В таких условиях Code Review необходим при изменении только ключевых компонентов продукта. В больших объёмах такой подход уже крайне неэффективен. Автоматизированная проверка кода статическими анализаторами поможет значительно выправить ситуацию в лучшую сторону. Это не замена существующим тестам, а совсем иной подход к контролю качества кода (к слову, в тестах тоже находятся ошибки с помощью статического анализа кода). Исправление ошибок во время написания кода почти ничего не стоит, в отличии от тех, которые находятся на этапе работы тестировщиков, а ошибки в выпущенном продукте несут уже колоссальные расходы.

Скачать и попробовать PVS-Studio можно по этой ссылке.

Обсудить варианты лицензирования, цены и варианты скидок можно написав нам в поддержку.

Не огорчайте единорога своим кодом…

Автор: PVS-Studio

Источник

Информация

Обсуждаемое

Рекомендуем

Серьёзные ошибки в коде CryEngine V

Введение

Несчастная функция Active()

Code above has no bugs

Ошибки при работе с памятью

Странный код

Про указатели

Проблемы с запятой

Подозрительные условия

Неопределённое поведение

Вопросы к разработчикам

Заключение

Архив

Информация

Обсуждаемое

Рекомендуем

Серьёзные ошибки в коде CryEngine V

Введение

Несчастная функция Active()

Code above has no bugs

Ошибки при работе с памятью

Странный код

Про указатели

Проблемы с запятой

Подозрительные условия

Неопределённое поведение

Вопросы к разработчикам

Заключение

Рекомендованный контент

Новости

Актуальные темы

Архив