Робота з помилкові спрацьовування в PVS-Studio і CppCat

    Handling False Positives
Нещодавно я вирішив знову перевірити фізичний движок Newton Game Dynamics. Код проекту якісний. Тому майже не було попереджень, що виявили помилки. Зате було кілька десятків помилкових спрацьовувань. Начебто писати статтю не про що. Але мені прийшла в голову думка, що можна написати про те, як працювати з помилкові спрацьовування, і як зробити, щоб їх не було. Проект Newton Game Dynamics здався мені підходящим для цього кандидатом.
 
 Перевірка проекту
Аналізатор PVS-Studio видає наступну кількість попереджень:
     
     
  • 48 першого рівня;
  •  
  • 79 другого рівня;

  •  
  • 261 третього рівня (вимкнені за замовчуванням).
  •  
Йдеться про попередження загального призначення (GA ).
 
Разом 127 попереджень, рекомендованих для перегляду. Рівно стільки ж повідомлень видає аналізатор CppCat . Далі в статті не робиться розходження між аналізаторами PVS-Studio і CppCat. В цілому, вони надають однакові механізми для придушення помилкових спрацьовувань. У PVS-Studio їх трохи більше, але загальної картини це не змінює.
 
Примітка. Чим різняться функціональні можливості PVS-Studio і CppCat можна тут .
 
Мені знадобилося трохи більше трьох годин, щоб виписати для статті всі потрібні приклади і позбутися від усіх попереджень. Думаю, якби я не виписував приклади, я б упорався за годину. Так що складність боротьби з помилковими спрацьовування перебільшена. Так, вони заважають і відволікають. Так, якщо проект великий, то і помилкових попереджень багато. Проте, від них зовсім не складно позбуватися.
 
Підсумок. CppCat видає 0 попереджень. PVS-Studio теж нічого не видає. Можна, звичайно, включити 3 рівень або 64-бітові діагностики, але це не так цікаво. Для початку дуже добре позбутися від рекомендованих попереджень. Це вже дуже великий крок до бік підвищення якості коду. З цього і треба почати. Якщо ви відразу включіть всі попередження, у вас не вистачить сил відразу дійти до кінця. Це, до речі, основна помилка новачків. «Більше» не означає «краще».
 
 Перегляд звіту
Аналізатори PVS-Studio і CppCat не об'єднують діагностичні повідомлення в групи і не сортують їх. При регулярному використанні це не потрібно. Якщо в новому коді аналізатор виявить 2-3 помилки, то об'єднувати і сортувати тут нічого. Тільки зайве ускладнення інтерфейсу.
 
Коли ви тільки починаєте працювати з інструментом, то можна відсортувати повідомлення за номером діагностики. Це робиться кліком мишки по заголовку стовпця з номером діагностики. Автоматично таке сортування ми робити не стали. Попередження виводяться в порядку аналізу файлів. Це дозволяє, не чекаючи зупинки аналізу, почати дивитися повідомлення. Якщо вони будуть сортуватися, то поки йде перевірка, повідомлення в таблиці будуть «стрибати», і працювати з ними буде неможливо.
 
Отже, на початкових етапах буде корисна сортування по типу попереджень (номеру діагностик). Я так і зроблю. Це дозволить мені швидко виявити однотипні помилкові спрацьовування і виключити їх. Це може істотно спростити роботу і скоротити час початкового налаштування.
 
 Робота з попередженнями
Якщо якийсь із способів придушення помилкових спрацьовувань здасться недостатньо зрозумілим, то пропонуємо познайомитися з відповідним розділом документації:  Попередження N1, N2
 
void
dgWorldDynamicUpdate::CalculateJointsVelocParallelKernel (....)
{
  ....
  dgVector velocStep2 (velocStep.DotProduct4(velocStep));
  dgVector omegaStep2 (omegaStep.DotProduct4(omegaStep));
  dgVector test ((velocStep2 > speedFreeze2) |
                 (omegaStep2 > omegaStep2));
  ....
}

Попередження: V501 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 Undefined behavior. Check the shift operator '<<. The left operand '(- 1LL)' is negative. dggoogol.cpp 249
 
Не можна зрушувати вліво від'ємні числа. Це призводить до невизначеного поведінки. Докладніше: " Не знаючи броду, не лізь у воду. Частина третя ".
 
Я виправив код наступним чином:
 
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 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 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 .

Я виправив код, використавши «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 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 The 'dgBaseNode' class implements a copy constructor, but lacks the the '=' operator. It is dangerous to use such a class. dgnode.h 35

Тут порушений " Закон Великий Двійки ". Є конструктор копіювання, але відсутній оператор копіювання (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 .

Примітка 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 .

Прочитали статтю і є питання? Часто до наших статей ставлять одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio і CppCat, версія 2014 . Будь ласка, ознайомтеся зі списком.


Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.