Недавно я решил вновь проверить физический движок Newton Game Dynamics. Код проекта качественный. Поэтому почти не было предупреждений, выявивших ошибки. Зато было несколько десятков ложных срабатываний. Вроде бы писать статью не о чем. Но мне пришла в голову мысль, что можно написать о том, как работать с ложными срабатываниями, и как сделать, чтобы их не было. Проект Newton Game Dynamics показался мне подходящим для этого кандидатом.
Проверка проекта
Анализатор PVS-Studio выдаёт следующее количество предупреждений:
|
Речь идёт о предупреждениях общего назначения (GA).
Итого 127 предупреждений, рекомендуемых для просмотра. Ровно столько же сообщений выдает анализатор CppCat. Далее в статье не делается различие между анализаторами PVS-Studio и CppCat. В целом, они предоставляют одинаковые механизмы для подавления ложных срабатываний. В PVS-Studio их чуть больше, но общей картины это не меняет.
Примечание. Чем различаются функциональные возможности PVS-Studio и CppCat можно здесь.
Мне потребовалось чуть больше трёх часов, чтобы выписать для статьи все нужные примеры и избавиться от всех предупреждений. Думаю, если бы я не выписывал примеры, я бы управился за час. Так что сложность борьбы с ложными срабатывания преувеличена. Да, они мешают и отвлекают. Да, если проект большой, то и ложных предупреждений много. Тем не менее, от них совсем не сложно избавляться.
Итог. CppCat выдаёт 0 предупреждений. PVS-Studio тоже ничего не выдаёт. Можно, конечно, включить 3 уровень или 64-битные диагностики, но это не так интересно. Для начала очень хорошо избавиться от рекомендуемых предупреждений. Это уже очень большой шаг к сторону повышения качества кода. С этого и надо начать. Если вы сразу включите все предупреждения, у вас не хватит сил сразу дойти до конца. Это, кстати, основная ошибка новичков. «Больше» не означает «лучше».
Просмотр отчёта
Анализаторы PVS-Studio и CppCat не объединяют диагностические сообщения в группы и не сортируют их. При регулярном использовании это не нужно. Если в новом коде анализатор обнаружит 2-3 ошибки, то объединять и сортировать здесь нечего. Только лишнее усложнение интерфейса.
Когда вы только начинаете работать с инструментом, то можно отсортировать сообщения по номеру диагностики. Это делается кликом мышки по заголовку столбца с номером диагностики. Автоматически такую сортировку мы делать не стали. Предупреждения выводятся в порядке анализа файлов. Это позволяет, не дожидаясь остановки анализа, начать смотреть сообщения. Если они будут сортироваться, то пока идёт проверка, сообщения в таблице будут «прыгать», и работать с ними будет невозможно.
Итак, на начальных этапах будет полезна сортировка по типу предупреждений (номеру диагностик). Я так и сделаю. Это позволит мне быстро выявить однотипные ложные срабатывания и исключить их. Это может существенно упростить работу и сократить время первоначальной настройки.
Работа с предупреждениями
Если какой-то из способов подавления ложных срабатываний покажется недостаточно понятным, то предлагаем познакомиться с соответствующим разделом документации:
- Для PVS-Studio — Подавление ложных предупреждений.
- Для CppCat — документация идёт в составе дистрибутива.
Предупреждение 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.
Автор: Andrey2008
