Недавно я решил вновь проверить физический движок Newton Game Dynamics. Код проекта качественный. Поэтому почти не было предупреждений, выявивших ошибки. Зато было несколько десятков ложных срабатываний. Вроде бы писать статью не о чем. Но мне пришла в голову мысль, что можно написать о том, как работать с ложными срабатываниями, и как сделать, чтобы их не было. Проект Newton Game Dynamics показался мне подходящим для этого кандидатом.

Проверка проекта

Анализатор PVS-Studio ^[1] выдаёт следующее количество предупреждений:

48 первого уровня; 79 второго уровня; 261 третьего уровня (выключены по умолчанию).

Речь идёт о предупреждениях общего назначения (GA ^[2]).

Итого 127 предупреждений, рекомендуемых для просмотра. Ровно столько же сообщений выдает анализатор CppCat ^[3]. Далее в статье не делается различие между анализаторами PVS-Studio и CppCat. В целом, они предоставляют одинаковые механизмы для подавления ложных срабатываний. В PVS-Studio их чуть больше, но общей картины это не меняет.

Примечание. Чем различаются функциональные возможности PVS-Studio и CppCat можно здесь ^[4].

Мне потребовалось чуть больше трёх часов, чтобы выписать для статьи все нужные примеры и избавиться от всех предупреждений. Думаю, если бы я не выписывал примеры, я бы управился за час. Так что сложность борьбы с ложными срабатывания преувеличена. Да, они мешают и отвлекают. Да, если проект большой, то и ложных предупреждений много. Тем не менее, от них совсем не сложно избавляться.

Итог. CppCat выдаёт 0 предупреждений. PVS-Studio тоже ничего не выдаёт. Можно, конечно, включить 3 уровень или 64-битные диагностики, но это не так интересно. Для начала очень хорошо избавиться от рекомендуемых предупреждений. Это уже очень большой шаг к сторону повышения качества кода. С этого и надо начать. Если вы сразу включите все предупреждения, у вас не хватит сил сразу дойти до конца. Это, кстати, основная ошибка новичков. «Больше» не означает «лучше».

Просмотр отчёта

Анализаторы PVS-Studio и CppCat не объединяют диагностические сообщения в группы и не сортируют их. При регулярном использовании это не нужно. Если в новом коде анализатор обнаружит 2-3 ошибки, то объединять и сортировать здесь нечего. Только лишнее усложнение интерфейса.

Когда вы только начинаете работать с инструментом, то можно отсортировать сообщения по номеру диагностики. Это делается кликом мышки по заголовку столбца с номером диагностики. Автоматически такую сортировку мы делать не стали. Предупреждения выводятся в порядке анализа файлов. Это позволяет, не дожидаясь остановки анализа, начать смотреть сообщения. Если они будут сортироваться, то пока идёт проверка, сообщения в таблице будут «прыгать», и работать с ними будет невозможно.

Итак, на начальных этапах будет полезна сортировка по типу предупреждений (номеру диагностик). Я так и сделаю. Это позволит мне быстро выявить однотипные ложные срабатывания и исключить их. Это может существенно упростить работу и сократить время первоначальной настройки.

Работа с предупреждениями

Если какой-то из способов подавления ложных срабатываний покажется недостаточно понятным, то предлагаем познакомиться с соответствующим разделом документации:

• Для PVS-Studio — Подавление ложных предупреждений ^[5].
• Для CppCat — документация идёт в составе дистрибутива.

Предупреждение N1, N2

void
dgWorldDynamicUpdate::CalculateJointsVelocParallelKernel (....)
{
  ....
  dgVector velocStep2 (velocStep.DotProduct4(velocStep));
  dgVector omegaStep2 (omegaStep.DotProduct4(omegaStep));
  dgVector test ((velocStep2 > speedFreeze2) |
                 (omegaStep2 > omegaStep2));
  ....
}

Предупреждение: V501 ^[6] There are identical sub-expressions to the left and to the right of the '>' operator: omegaStep2 > omegaStep2 dgworlddynamicsparallelsolver.cpp 546

Выражение «omegaStep2 > omegaStep2» выглядит подозрительно. Мне сложно судить, есть здесь ошибка или нет. Так как это сравнение встречается и в другом файле, то, наверное, это всё же не ошибка, а так и задумано.

Пусть будет не ошибка. Я разметил эти два места с помощью комментария:

dgVector test ((velocStep2 > speedFreeze2) |
               (omegaStep2 > omegaStep2)); //-V501

Теперь, в этом месте предупреждение V501 выдаваться не будет.

Предупреждение N3

dgInt32
dgWorld::CalculatePolySoupToHullContactsDescrete(....) const
{
  ....
  dgAssert (dgAbsf(polygon.m_normal % polygon.m_normal -
            dgFloat32 (1.0f)) < dgFloat32 (1.0e-4f));
  ....
}

Предупреждение: V501 There are identical sub-expressions to the left and to the right of the '%' operator: polygon.m_normal % polygon.m_normal dgnarrowphasecollision.cpp 1921

Анализатор здесь одновременно прав и неправ. С одной стороны, выражение «polygon.m_normal % polygon.m_normal» действительно очень подозрительно. Но анализатор не догадывается, что это тест для проверки реализованного в классе оператора '%'. На самом деле, код корректен. Поможем анализатору комментарием:

dgAssert (dgAbsf(polygon.m_normal % polygon.m_normal - //-V501
          dgFloat32 (1.0f)) < dgFloat32 (1.0e-4f));

Предупреждение N4

static void PopupateTextureCacheNode (dScene* const scene)
{
  ....
  if (!(info->IsType(dSceneCacheInfo::GetRttiType()) ||
        info->IsType(dSceneCacheInfo::GetRttiType()))) {
  ....
}

Предупреждение: V501 There are identical sub-expressions 'info->IsType(dSceneCacheInfo::GetRttiType())' to the left and to the right of the '||' operator. dscene.cpp 125

Два раза проверяется одно и тоже. Будем считать, что вторая проверка лишняя. Поэтому я исправил код следующим образом:

if (!(info->IsType(dSceneCacheInfo::GetRttiType()))) {

Предупреждение N5

dFloat dScene::RayCast (....) const
{
  ....
  dFloat den = 1.0f /
    ((globalP1 - globalP0) % (globalP1 - globalP0)); //-V501
  ....
}

Предупреждение: V501 There are identical sub-expressions '(globalP1 — globalP0)' to the left and to the right of the '%' operator. dscene.cpp 1280

Переменные globalP0 и globalP1 являются экземплярами класса 'dVector'. Поэтому код имеет смысл. Анализатор зря беспокоится. Разметим код:

dFloat den = 1.0f /
  ((globalP1 - globalP0) % (globalP1 - globalP0)); //-V501

Хотя анализатор не прав, красивым этот код назвать нельзя. Думаю, можно завести специальные функции для таких случаев или что-то ещё.

Предупреждение N6 — N15

dgInt32 dgCollisionCompound::CalculateContactsToCompound (
  ...., dgCollisionParamProxy& proxy) const
{
  ....
  dgCollisionInstance childInstance
    (*subShape, subShape->GetChildShape());
  ....
  proxy.m_referenceCollision = &childInstance; 
  ....
  m_world->CalculateConvexToConvexContacts(proxy);
  ....
}

Предупреждение: V506 ^[7] Pointer to local variable 'childInstance' is stored outside the scope of this variable. Such a pointer will become invalid. dgcollisioncompound.cpp 1815

В функцию приходит ссылка на объект типа 'dgCollisionParamProxy'. В этот объект записывается указатель на локальную переменную. Анализатор предупреждает, что это потенциально опасно. По выходу из функции этот указатель использовать будет нельзя, так как локальная переменная будет разрушена.

В данном случае никакой ошибки нет. Указатель используется только тогда, когда переменная существует.

Подавлять комментариями такие предупреждения не хочется. Дело в том, что есть ещё 9 однотипных предупреждений.

Поступим другим образом. Во всех строках, где выдаётся ложное предупреждение, фигурирует переменная с именем 'proxy'. Мы можем написать один единственный комментарий, который подавит все эти предупреждения:

//-V:proxy:506

Его нужно вписать в какой-то файл, который включается во все другие файлы. В нашем случае, оптимальным для этого является файл «dgPhysicsStdafx.h».

Теперь для тех строк, где встречается слово 'proxy', не будет выдаваться предупреждение V506. Изначально этот механизм был создан для подавления предупреждений в макросах. Но, на самом деле, всё равно, слово означает макрос или что-то ещё (переменную, функцию, имя класса и т.д.). Принцип прост. Если в строке встречается указанная подстрока, то соответствующее предупреждение не выводится.

Предупреждение N16

Длинный пример. Можете пропустить. Ничего интересного не потеряете.

Есть класс вектора:

class dgVector
{
  ....
  union {
    __m128 m_type;
    __m128i m_typeInt;
    dgFloat32 m_f[4];
    struct {
      dgFloat32 m_x;
      dgFloat32 m_y;
      dgFloat32 m_z;
      dgFloat32 m_w;
    };
    struct {
      dgInt32 m_ix;
      dgInt32 m_iy;
      dgInt32 m_iz;
      dgInt32 m_iw;
    };
  };
  ....
};

И есть вот такой код, где члены вектора заполняются значениями с помощью функции memcpy():

DG_INLINE dgMatrix::dgMatrix (const dgFloat32* const array)
{
  memcpy (&m_front.m_x, array, sizeof (dgMatrix)) ;
}

Предупреждение: V512 ^[8] A call of the 'memcpy' function will lead to overflow of the buffer '& m_front.m_x'. dgmatrix.h 118

Анализатору не нравится, что в переменную типа 'dgFloat32' копируется больше байт, чем она занимает. Не очень красивый, но работающий и широко используемый приём. На самом деле, заполнится переменная m_x, m_y, m_z и так далее.

В начале я был невнимателен и поправил код следующим образом:

memcpy(m_front.m_f, array, sizeof(dgMatrix));

Я подумал, что копируется только один вектор. А размер массива 'm_f' как раз совпадает с размером вектора.

Но при последующем запуске анализатор вновь одёрнул меня. На самом деле, копируется не один вектор, а 4 вектора. Именно 4 вектора содержит класс 'dgMatrix':

class dgMatrix
{
  ....
  dgVector m_front;
  dgVector m_up;
  dgVector m_right;
  dgVector m_posit;
  ....
}

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

memcpy (&m_front.m_x, array, sizeof (dgMatrix)) ; //-V512

Предупреждение N17, N18

void dgWorldDynamicUpdate::UpdateDynamics(dgFloat32 timestep)
{
  dgWorld* const world = (dgWorld*) this;
  dgUnsigned32 updateTime = world->m_getPerformanceCount();

  m_bodies = 0;
  m_joints = 0;
  m_islands = 0;
  m_markLru = 0;

  world->m_dynamicsLru = world->m_dynamicsLru + DG_BODY_LRU_STEP;
  m_markLru = world->m_dynamicsLru;
  ....
}

Предупрежжение: V519 ^[9] The 'm_markLru' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 91, 94. dgworlddynamicupdate.cpp 94

В переменную 'm_markLru' в начале записывается 0, а затем 'world->m_dynamicsLru'. Ошибки здесь никакой нет. Чтобы избавиться от предупреждения, я удалил инициализацию переменной нулём.

Аналогично я поступил ещё в одном месте. Соответствующее предупреждение:

V519 The 'm_posit' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1310, 1313. customvehiclecontrollermanager.cpp 1313

Предупреждение N19, N20

dgFloat32 dgCollisionConvexPolygon::GetBoxMinRadius () const
{
  return m_faceClipSize;
}

dgFloat32 dgCollisionConvexPolygon::GetBoxMaxRadius () const
{
  return m_faceClipSize;  
}

Предупреждение: V524 ^[10] It is odd that the body of 'GetBoxMaxRadius' function is fully equivalent to the body of 'GetBoxMinRadius' function. dgcollisionconvexpolygon.cpp 88

Две функции, содержащие в своём названии 'Min' и 'Max', устроены одинаковым образом. Для анализатора это подозрительно. На самом деле, ошибки здесь нет. Чтобы устранить ложное срабатывание, я реализовав одну функцию через другую:

dgFloat32 dgCollisionConvexPolygon::GetBoxMaxRadius () const
{
  return GetBoxMinRadius();
}

Аналогичным образом я поступил с функциями GetBoxMaxRadius/GetBoxMaxRadius, реализованными в классе 'dgCollisionScene'.

Предупреждение N21

dgInt32 AddFilterFace (dgUnsigned32 count, dgInt32* const pool)
{
  ....
  for (dgUnsigned32 i = 0; i < count; i ++) {
    for (dgUnsigned32 j = i + 1; j < count; j ++) {
      if (pool[j] == pool[i])
      {
        for (i = j; i < count - 1; i ++) {
          pool[i] =  pool[i + 1];
        }
        count --;
        i = count;
        reduction = true;
        break;
      }
    }
  }
  ....
}

Предупреждение: V535 ^[11] The variable 'i' is being used for this loop and for the outer loop. Check lines: 105, 108. dgpolygonsoupbuilder.cpp 108

Есть два цикла. Один использует в качестве счётчика переменную 'i', другой 'j'. Внутри этих циклов иногда запускается ещё один цикл. В качестве счётчика он вновь использует переменную 'i'. Анализатору это не нравится, хотя ошибки здесь нет.

Если выполняется внутренний цикл, то внешние циклы останавливаются:

• цикл, организованный с помощью переменной 'j', останавливается благодаря оператору 'break';
• цикл, организованный с помощью переменной 'i', останавливается благодаря присваиванию «i = count».

Анализатор не смог разобраться в таком хитросплетении. Это пример работающего кода, который тем не менее попахивает.

Чтобы устранить ложное срабатывание, я использовал комментарий:

for (i = j; i < count - 1; i ++) { //-V535

Предупреждение N22 — N25

DG_INLINE dgMatrix::dgMatrix (const dgVector& front)
{
  ....
  m_right = m_right.Scale3 (dgRsqrt (m_right % m_right));
  m_up = m_right * m_front;
  ....
}

Предупреждение: V537 ^[12] Consider reviewing the correctness of 'm_right' item's usage. dgmatrix.h 143

Предупреждение V537 выводится, когда подозрительно смешиваются переменные, в именах которых есть «right», «left», «front» и так далее. Для данного проекта данная диагностика оказалась неудачной. Анализатор выдал 4 предупреждения на совершенно безобидный код.

В таком случае, в PVS-Studio можно полностью отключить в настройках диагностику V537.

В CppCat нельзя отключать отдельные диагностики. Воспользуемся альтернативным подходом. Во всех строках, где были ложные предупреждения, присутствует слово «right». Я добавил в файл «dgStdafx.h» комментарий:

//-V:right:537

Предупреждение N26

Обратите внимание на комментарий.

int pthread_delay_np (struct timespec *interval)
{
  ....
  /*
   * Most compilers will issue a warning 'comparison always 0'
   * because the variable type is unsigned,
   * but we need to keep this
   * for some reason I can't recall now.
   */
  if (0 > (wait_time = secs_in_millisecs + millisecs))
  {
    return EINVAL;
  }
  ....
}

Предупреждение: V547 ^[13] Expression is always false. Unsigned type value is never < 0. pthread_delay_np.c 119

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

if (0 > (wait_time = secs_in_millisecs + millisecs)) //-V547

Предупреждение N27

typedef unsigned long long dgUnsigned64;

dgUnsigned64 m_mantissa[DG_GOOGOL_SIZE];

dgGoogol::dgGoogol(dgFloat64 value)
  :m_sign(0)
  ,m_exponent(0)
{
  ....
  m_mantissa[0] = (dgInt64 (dgFloat64 (
                    dgUnsigned64(1)<<62) * mantissa));

  // it looks like GCC have problems with this
  dgAssert (m_mantissa[0] >= 0);
  ....
}

Предупреждение: V547 Expression 'm_mantissa[0] >= 0' is always true. Unsigned type value is always >= 0. dggoogol.cpp 55

Анализатор разделяет мнение GCC, что с этим кодом что-то не в порядке (см. комментарий в коде).

Проверка «dgAssert(m_mantissa[0] >= 0)» не имеет смысла. Беззнаковая переменная всегда больше или равна нулю. Имеющийся в коде 'dgAssert' ничего, на самом деле, не проверяет.

Программисты бывают ленивы. Вместо того, чтобы разобраться и исправить ошибку, они пишут комментарий.

Я исправил код так, чтобы 'dgAssert' осуществлял нужную проверку. Для этого понадобилось завести временную знаковую переменную:

dgInt64 integerMantissa = (dgInt64(dgFloat64(
                             dgUnsigned64(1) << 62) * mantissa));
dgAssert(integerMantissa >= 0);
m_mantissa[0] = integerMantissa;

Предупреждение N28 — N31

void dgRedBackNode::RemoveFixup (....) 
{
  ....
  if (!ptr) {
    return;
  }
  ....
  ptr->SetColor(RED) ;
  ptr->RotateLeft (head);
  tmp = ptr->m_right;
  if (!ptr || !tmp) {
    return;
  }
  ....
}

Предупреждение: V560 ^[14] A part of conditional expression is always false: !ptr. dgtree.cpp 215

Выражение '!ptr' всегда ложно. Дело в том, что указатель 'ptr' уже проверялся ранее на равенство нулю. Если указатель равен нулю, происходил выход из функции.

Вторая проверка смотрится ещё более глупо из-за того, что указатель перед ней разыменовывается: «tmp = ptr->m_right;».

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

if (!ptr) {
  return;
}
....
tmp = ptr->m_right;
if (!tmp) {
  return;
}
....

Аналогичным образом исправил ещё 3 фрагмента кода.

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

Предупреждение N32, N33

DG_INLINE bool dgBody::IsCollidable() const;

void dgBroadPhase::AddPair (dgBody* const body0, dgBody* const body1, const dgVector& timestep2, dgInt32 threadID)
{
  ....
  bool kinematicBodyEquilibrium =
    (((body0->IsRTTIType(dgBody::m_kinematicBodyRTTI) ?
      true : false) & body0->IsCollidable()) |
    ((body1->IsRTTIType(dgBody::m_kinematicBodyRTTI) ?
      true : false) & body1->IsCollidable())) ? false : true;
  ....
}

Предупреждение: V564 ^[15] The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. dgbroadphase.cpp 921

Код с «душком». Непонятно, зачем было писать такую сложную и непонятную проверку. Я переписал её. Код стал немного короче и легче для чтения. Плюс пропало предупреждение анализатора.

bool kinematicBodyEquilibrium =
  !((body0->IsRTTIType(dgBody::m_kinematicBodyRTTI) &&
     body0->IsCollidable()) ||
    (body1->IsRTTIType(dgBody::m_kinematicBodyRTTI) &&
     body1->IsCollidable()));

Было выдано ещё одно предупреждение V564, где я тоже упростил код:

V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. dgbroadphase.cpp 922

Предупреждение N34 — N37

class dgAIWorld: public dgAIAgentGraph { .... };

typedef struct NewtonAIWorld{} NewtonAIWorld;

NewtonAIWorld* NewtonAICreate ()
{
  TRACE_FUNCTION(__FUNCTION__);
  dgMemoryAllocator* const allocator = new dgMemoryAllocator();
  NewtonAIWorld* const ai = (NewtonAIWorld*)
    new (allocator) dgAIWorld (allocator);
  return ai;
}

Предупреждение: V572 ^[16] It is odd that the object which was created using 'new' operator is immediately casted to another type. newtonai.cpp 40

Странный способ хранения объектов. Создаём объект класса 'dgAIWorld'. Явно приводим его к типу ' NewtonAIWorld'. Не стал разбираться, зачем это сделано. Видимо, это для чего-то нужно. Просто подавил предупреждение с помощью комментариев в этой и ещё в 3 функциях.

Предупреждение N38

void dgCollisionCompound::EndAddRemove ()
{
  ....
  if (node->m_type == m_node) {
    list.Append(node);
  }

  if (node->m_type == m_node) {
    stack.Append(node->m_right);
    stack.Append(node->m_left);
  } 
  ....
}

Предупреждение: V581 ^[17] The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 952, 956. dgcollisioncompound.cpp 956

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

if (node->m_type == m_node) {
  ....
}
if (node->m_type == m_FOO) {
  ....
} 

В обнаруженном коде всё корректно. Чтобы избавиться от ложного срабатывания, лучше всего исправить код. Как мне кажется, я не изменю логику работы программы, сделав только одну проверку:

if (node->m_type == m_node) {
  list.Append(node);
  stack.Append(node->m_right);
  stack.Append(node->m_left);
}

Предупреждение N39

void dSceneGraph::AddEdge (....)
{
  ....
  if ((!parentLink && !childLink)) {
  ....
}

Предупреждение: V592 ^[18] The expression was enclosed by parentheses twice: '((!parentLink &&!childLink))'. One pair of parentheses is unnecessary or misprint is present. dscenegraph.cpp 209

Ничего страшного. Просто лишние скобки. Удалил их:

if (!parentLink && !childLink) {

Предупреждение N40 — N44

dgVector dgCollisionCylinder::SupportVertex (....) const
{
  dgAssert (dgAbsf ((dir % dir - dgFloat32 (1.0f))) <
            dgFloat32 (1.0e-3f));
  ....
}

Предупреждение: V592 The expression was enclosed by parentheses twice: '((dir % dir — dgFloat32(1.0f)))'. One pair of parentheses is unnecessary or misprint is present. dgcollisioncylinder.cpp 202

Ничего страшного. Просто лишние скобки. Удалил их, чтобы не смущать анализатор:

dgAssert (dgAbsf (dir % dir - dgFloat32 (1.0f)) <
          dgFloat32 (1.0e-3f));

Эта строка растиражирована с помощью Copy-Paste ещё в 4 фрагмента кода. Там я тоже убрал лишнюю пару скобок.

Предупреждение N45 — N65

void
ptw32_throw (DWORD exception)
{
  ....
  ptw32_thread_t * sp =
    (ptw32_thread_t *) pthread_getspecific (ptw32_selfThreadKey);

  sp->state = PThreadStateExiting;

  if (exception != PTW32_EPS_CANCEL &&
      exception != PTW32_EPS_EXIT)
  {
    exit (1);
  }
  ....
  if (NULL == sp || sp->implicit)
  ....
}

Предупреждение: V595 ^[19] The 'sp' pointer was utilized before it was verified against nullptr. Check lines: 77, 85. ptw32_throw.c 77

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

Здесь как раз такой случай. В начале переменная 'sp' разыменовывается в выражении «sp->state». Затем она проверяется на равенство NULL.

Анализатор обнаружил ещё 20 подобных фрагментов кода. В каждом конкретном случае следует поступать по-разному. Где-то я перенёс проверку до разыменовывания. Где-то просто удалил её.

Примечание

Очень часто причиной ложного предупреждение V595 является макрос, похожий на это:

#define FREE(p) { if (p) free(p); }

Конкретно в этом случае анализатор должен понять, что имел в виду программист и промолчит. Но в общем случае могут возникать ложные срабатывания на вот таком коде:

p->foo();
FREE(p);

В таких случаях я рекомендую избавляться от макросов. Приведённый выше макрос FREE() совершенно бессмысленный и вредный.

Во-первых, нет смысла проверять указатель на равенство нулю. Функция free() корректно работает с нулевыми указателями. С оператором 'delete' тоже самое. Поэтому макрос FREE() не нужен. Совсем.

Во-вторых, он опасен. Если мы будем извлекать указатели из массива, то это может привести к ошибке. Пример: FREE(ArrayOfPtr[i++]); — будет проверен один указатель, а освобождён следующий.

Предупреждение N66

void dgCollidingPairCollector::Init ()
{
  dgWorld* const world = (dgWorld*) this;
  // need to expand teh buffer is needed 
  world->m_pairMemoryBuffer[0];
  m_count = 0;
}

Предупреждение: V607 ^[20] Ownerless expression 'world->m_pairMemoryBuffer[0]'. dgcontact.cpp 342

Комментарий подсказывает нам, что выражение «world->m_pairMemoryBuffer[0]» имеет смысл. Анализатор про это не знает и выдаёт ложное предупреждение. Я устранил его, используя разметку кода:

world->m_pairMemoryBuffer[0]; //-V607

Более красивым решением добавить специальный метод, расширяющий буфер. Тогда код выглядил бы как-то так:

void dgCollidingPairCollector::Init ()
{
  dgWorld* const world = (dgWorld*) this;
  world->m_pairMemoryBuffer.ExpandBuffer();
  m_count = 0;
}

Теперь комментарий не нужен. Код говорит за себя сам. Анализатор не выдаёт предупреждений. Идиллия.

Предупреждение N67

dgGoogol dgGoogol::Floor () const
{
  ....
  dgUnsigned64 mask = (-1LL) << (64 - bits);
  ....
}

Предупреждение: V610 ^[21] Undefined behavior. Check the shift operator '<<. The left operand '(- 1LL)' is negative. dggoogol.cpp 249

Нельзя сдвигать влево отрицательные числа. Это приводит к неопределённому поведению. Подробнее: "Не зная брода, не лезь в воду. Часть третья ^[22]".

Я исправил код следующим образом:

dgUnsigned64 mask = (~0LLU) << (64 - bits);

Предупреждение N68 — N79

void dGeometryNodeSkinModifierInfo::RemoveUnusedVertices(
  const int* const vertexMap)
{
  ....
  dVector* vertexWeights = new dVector[m_vertexCount];
  dBoneWeightIndex* boneWeightIndex =
                           new dBoneWeightIndex[m_vertexCount];
  ....
  delete boneWeightIndex;
  delete vertexWeights;
}

Предупреждения:

• V611 ^[23] The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] boneWeightIndex;'. dgeometrynodeskinmodifierinfo.cpp 97
• V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] vertexWeights;'. dgeometrynodeskinmodifierinfo.cpp 98

Забыты квадратные скобки у операторов 'delete'. Это ошибки, которые следует исправить. Корректный вариант кода:

delete [] boneWeightIndex;
delete [] vertexWeights;

Анализатор нашёл ещё 10 таких мест, и везде я исправил код.

Предупреждение N80

#if defined(_MSC_VER)
/* Disable MSVC 'anachronism used' warning */
#pragma warning( disable : 4229 )
#endif

typedef void (* PTW32_CDECL ptw32_cleanup_callback_t)(void *);

#if defined(_MSC_VER)
#pragma warning( default : 4229 )
#endif

Предупреждение: V665 ^[24] Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 733, 739. pthread.h 739

Это плохой способ подавления предупреждений. Особенно, если это код библиотеки. В чем причина и, как сделать лучше, можно узнать из описания диагностики V665 ^[24].

Я исправил код, использовав «warning(push)» и " warning(pop)":

#if defined(_MSC_VER)
/* Disable MSVC 'anachronism used' warning */
#pragma warning( push )
#pragma warning( disable : 4229 )
#endif

typedef void (* PTW32_CDECL ptw32_cleanup_callback_t)(void *);

#if defined(_MSC_VER)
#pragma warning( pop )
#endif

Предупреждение N81 — N99

dgAABBPointTree4d* dgConvexHull4d::BuildTree (....) const
{
  ....
  const dgBigVector& p = points[i];
  ....
  varian = varian + p.CompProduct4(p);
  ....
}

Предупреждение: V678 ^[25] An object is used as an argument to its own method. Consider checking the first actual argument of the 'CompProduct4' function. dgconvexhull4d.cpp 536

Анализатору не нравятся вызовы функций следующего вида: X.Foo(X). Во-первых, это может быть опечаткой. Во-вторых, класс может оказаться не готов к тому, что ему придётся работать с самим собой.

В данном случае код корректен. Ложное предупреждение нужно подавить. Можно поставить комментарий вида:

varian = varian + p.CompProduct4(p); //-V678

Это является плохой идеей. Анализатор выдал ещё 18 таких предупреждений, и не хочется добавлять в код так много комментариев.

К счастью, все 19 предупреждений относятся к вызовам функций CompProduct3() или CompProduct4. Поэтому можно написать один комментарий, который подавит все предупреждения V678 в строках, где содержится подстрока «CompProduct»:

//-V:CompProduct:678

Я разместил этот комментарий в файле dgStdafx.h.

Предупреждение N100 — N119

Класс 'dgBaseNode' содержит указатели:

class dgBaseNode: public dgRef
{
  ....
  dgBaseNode (const dgBaseNode &clone);
  ....
private:
  ....
  dgBaseNode* parent;
  dgBaseNode* child;
  dgBaseNode* sibling;
};

Поэтому в нём реализован полноценный конструктор копирования:

dgBaseNode::dgBaseNode (const dgBaseNode &clone)
  :dgRef (clone)
{
  Clear ();

  for (dgBaseNode* obj = clone.child; obj; obj = obj->sibling) {
    dgBaseNode* newObj = (dgBaseNode *)obj->CreateClone ();
    newObj->Attach (this);
    newObj->Release();
  }
}

Предупреждение: V690 ^[26] The 'dgBaseNode' class implements a copy constructor, but lacks the the '=' operator. It is dangerous to use such a class. dgnode.h 35

Здесь нарушен "Закон Большой Двойки ^[27]". Есть конструктор копирования, но отсутствует оператор копирования (operator =). В результате, при присваивании компилятор просто скопирует значения указателей, что приведёт к трудноуловимым ошибкам. Даже если сейчас оператор копирования не используется, этот код потенциально опасен. Очень легко сделать ошибку.

Правильный вариант действия здесь один — реализовать operator =. Если этот оператор по смыслу не нужен, то можно объявить его в приватной секции.

Анализатор нашёл ещё 18 классов, где забыли реализовать (или запретить) оператор копирования.

Ещё есть один странный класс, смысл и назначения которого мне не понятен:

struct StringPool
 {
  char buff[STRING_POOL_SIZE];
  StringPool ()
  {
  }
  StringPool (const StringPool &arg)
  {
  }
};

Здесь я просто подавил ложное срабатывание с помощью комментария:

struct StringPool //-V690
{
  ....
};

Примечание 1. В C++11 появились новые ключевые слова, которые упрощают запрет использования конструкторов копирования, операторов присваивания. Или говорят, что конструктор копирования или оператор присваивания, созданный компилятором по умолчанию, являются корректными. Речь идёт о =default и =delete. Подробнее о них можно прочитать, например, в C++FAQ ^[28].

Примечание 2. Я часто вижу, что во многих программах реализован оператор копирования или присваивания, хотя он не нужен. Я имею в виду ситуации, когда объект отлично может быть скопирован компилятором. Простой искусственный пример:

struct Point {
  int x, y;
  Point &Point(const Point &p) { x = p.x; y = p.y; return *this; }
};

Имеется никому ненужный оператор копирования. Нарушено правило «Закон Большой Двойки», и анализатор выдаёт предупреждение. Чтобы не писать ещё одну ненужную функцию (конструктор копирования), надо удалить operator =.

Отличный короткий правильный класс:

struct Point {
  int x, y;
};

Предупреждение N120 — N125

Осталось ещё 6 предупреждений разного типа. Я с ними не справился. Код проекта мне совершенно незнаком. Не могу понять, имею я дело с ошибкой или ложным срабатыванием. Плюс, если даже это ошибка, мне всё равно не понятно, как нужно изменить код. Не стал себя мучить и просто разметил с помощью комментариев эти предупреждения, как ложные.

Предупреждение N126 — N127

Два предупреждения «потерялись». Это нормально. Дело в том, что иногда один и тот же подозрительный фрагмент кода может приводить к появлению 2, а иногда и 3 предупреждений. Соответственно, одной правкой может исправляться несколько предупреждений. Например, предупреждения V595 могли исчезнуть в процессе правок кода, связанных с V560 (см предупреждения N28 — N31).

Выводы

Как видите, совсем ложных срабатываний очень мало. Много сообщений указывают на код, «который пахнет». Да, этот код корректно работает. Тем не менее он странен, его тяжело читать и поддерживать. То, что может сбить с толку анализатор, может сбить с толку и человека.

Часто фрагменты кода, на которые указал анализатор, можно переписать. Это не только устранит предупреждение, но и сделает код более понятным.

В том случае, когда анализатор действительно не прав, есть различные методы подавления ложных предупреждений. Подробно они рассмотрены в документации.

Надеюсь, мне удалось показать в статье, что работа с ложными срабатываниями не так сложна, как может показаться с первого взгляда. Желаю удачи в использовании наших статических анализаторов кода.

Эта статья на английском

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Handling False Positives in PVS-Studio and CppCat ^[29].

Автор: Andrey2008

Источник ^[31]