Введение

Опыт чрезвычайно важен во всех сферах деятельности, и программирование - не исключение. Где этот опыт получить? Читать соответствующую литературу, писать код, анализировать. И если до первого и второго руки доходят, то до последнего добраться никак не удавалось. Конечно, периодически необходимо просматривать свой код с целью поиска мест для улучшений, но, пожалуй, не менее важно читать и анализировать другой код. Для этих целей как нельзя лучше подходят open-source проекты. А если под рукой есть удобный статический анализатор - процесс идёт быстрее и интереснее, ввиду того что поиск и анализ "узких" мест программы значительно увеличивается.

Таким анализатором оказался PVS-Studio. Кто-то с ним уже знаком, для остальных хотелось бы рассказать о нём в общих чертах.

PVS-Studio - статический анализатор кода, разрабатываемый компанией ООО "СиПроВер". Предназначен он для выявления различного рода ошибок: общего назначения и 64-битных. Имеет обширную справочную систему, позволяющую лучше вникнуть в суть ошибок и, например, открыть для себя что-то новое (воистину, век живи - век учись). Более подробно останавливаться не буду - при желании сможете прочитать на официальном сайте анализатора.


Об анализируемом проекте

Естественно, для анализа необходимо выбрать соответствующий проект. В сети достаточно много open-source проектов, написанных с использованием C/C++, в то же время немало проектов уже было проверено. Одним из непроверенных оказался Stellarium. Stellarium - это свободный виртуальный планетарий, с открытым исходным кодом, доступный в соответствии с GNU для платформ Linux, Mac OS X, Microsoft Windows, Symbian, Android и iOS (в последних трех как Stellarium Mobile).  Думаю, что программа весьма распространена в определённых кругах. Оно и оправдано: достаточная функциональность, удобный интерфейс. Сам неоднократно пользовался данным продуктом, в том числе для наблюдения за различными небесными телами, но здесь немного не о том. Кроме самого проекта были проверены несколько плагинов, входящих в его состав (например, Stellarium: Meteor Showers, Stellarium Telescope Control). 

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


Анализ проекта

Предупреждение анализатора: V501 There are identical sub-expressions to the left and to the right of the '<=' operator: d1 <= d1 octahedronpolygon.cpp 39



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

Предупреждение анализатора: V501 There are identical sub-expressions 'cap.intersects(cap2)' to the left and to the right of the '&&' operator. stelprojectorclasses.hpp 224



Ситуация  аналогична предыдущей. В логическом выражении используются 2 одинаковых подвыражения - cap.intersects(cap2). Стоит отметить, что подобный код встречался несколько раз. Оно неудивительно - какое программирование без копипаста? Впрочем, данное выражение всего лишь избыточно, так что подобная ошибка не столь страшна.

Предупреждение анализатора: V562 It's odd to compare a bool type value with a value of float type: MeteorShower::showLabels != b. meteorshowers.cpp 1222



Такая ситуация потенциально опасна, ввиду того, что число с плавающей точкой сравнивается с типом bool. Возможно, лучшим решением было бы всё же объявить переменную 'showLabels' типа bool.

Предупреждение анализатора: V570 The 'serialDeviceName' variable is assigned to itself. telescopeclientdirectnexstar.cpp 79



Как видно из кода, в альтернативной ветви условного оператора выполняется присвоение переменной 'serialDeviceName' самой себе, при том без каких-либо изменений, что как минимум странно. Подобный код встретился 2 раза.

Предупреждение анализатора: V808 'tn' object of 'QString' type was created but was not utilized. telescopecontrol.cpp 420



В данном методе дважды объявляется переменная 'tn' типа QString, причём в разных областях видимости: одна - внутри цикла, другая вне его. Отсюда следует сразу два тонких момента:

• Первая переменная никаким образом не используется. Следовательно, просто было потрачено время на создание объекта, а также память на его хранение.
• Переменная 'tn' объявлена внутри цикла. Следовательно, на каждой итерации цикла будут вызываться конструктор и деструктор (не забываем, что 'tn' - объект). Лишние временные затраты, которые ни к чему.

Предупреждение анализатора: V808 'tmpstr' object of 'QString' type was created but was not utilized. constellationmgr.cpp 458



Ничего особо интересного, просто создание переменной, которая нигде не используется. Тем не менее, память и время на вызов конструктора опять-таки расходуем.

Предупреждение анализатора: V815 Decreased performance. Consider replacing the expression 'c.modulation != ""' with '!c.modulation.isEmpty()'. satellite.cpp 196



Подобный код встречается неоднократно. Левая и правая части условного выражения идентичны, так что одну можно убрать (например, правую, т.к. вызов метода isEmpty() будет эффективнее). Анализатор нашёл 21 подобный участок кода.

Предупреждение анализатора: V815 Decreased performance. Consider replacing the expression 'shortOpt != ""' with '!shortOpt.isEmpty()'. cliprocessor.cpp 272



Проверку 'shortOpt' на пустую строку можно было бы выполнить с помощью метода isEmpty(). Но здесь, как говорится, бабушка надвое сказала. Если данное сравнение не используется в цикле, скорее всего прирост производительности не будет сильно заметным, так что тут каждый выбирает для себя, ввиду того, что вариант shortOpt!="" кому-то покажется более наглядным.

Предупреждение анализатора: V509 The 'new' operator should be located inside the try..catch block, as it could potentially generate an exception. Raising exception inside the destructor is illegal. stelapp.cpp 265



Оператор 'new' отмечен не случайно. Дело в том, что если в программе возникает исключение, начинается свёртывание стека, в ходе которого объекты разрушаются путём вызова деструкторов. Но если деструктор бросает ещё одно исключение ('new' сгенерирует исключение, если не удастся выделить память) и это исключение покинет деструктор, библиотека C++ аварийно завершит программу, вызвав функцию terminate(). Для того, чтобы избежать подобных ситуаций, исключение, сгенерированное внутри деструктора, должно быть обработано там же.

Предупреждение анализатора: V636 The 'prj->getViewportWidth() / 2' expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. pointercoordinates.cpp 122



В чём причина потенциальной ошибки? Дело в том, что методы getViewportWidth() и getViewportHeight() возвращают значения типа int. В итоге выполняется целочисленное деление, результат которого, однако, неявно приводится к типу float. Возможно, именно поэтому анализатор и указал на данный код. Анализатором было отмечено ещё 12 подобных участков.

Предупреждение анализатора: V612 An unconditional 'break' within a loop. stelmainview.cpp 426



Имеется очень подозрительный цикл, у которого будет выполняться только одна итерация, так как вне зависимости от выполнения условия внутри цикла, дальше будет вызван оператор 'break'. Возможно, этот оператор следовало поместить внутрь блока, относящегося к 'if'.

Предупреждение анализатора: V652 The '!' operation is executed 3 or more times in succession. Consider inspecting the '!!!checked' expression. stelguiitems.cpp 141



Тройное повторение оператора '!' выглядит подозрительно.

Предупреждение анализатора: V668 There is no sense in testing the 'sPainter' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. comet.cpp 541



Как видно из сообщения анализатора, проверка на то, является ли указатель 'sPainter' нулевым, не имеет смысла. Дело в том, что если оператору 'new' не удастся выделить память, будет сгенерировано исключение std::bad_alloc(), так что проверять указатель на равенство нулю не имеет смысла. Анализатором было выявлено ещё 10 участков подобного кода.

Предупреждение анализатора: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'pq->keys' is lost. Consider assigning realloc() to a temporary pointer. priorityq.c 228



Объявление 'memRealloc' выглядит следующим образом:



То есть, фактически, это та же функция 'realloc'.

Данная конструкция потенциально опасна по следующим причинам. Если функции 'realloc' удастся изменить размер блока памяти, не прибегая к перемещению данных, - то результирующий указатель совпадёт с исходным (pp->keys). Если необходимо переместить данные, старый указатель будет освобождён, а функция вернёт указатель на новый блок памяти. Но если не удастся изменить размер блока памяти, функция вернёт нулевой указатель, и тогда указатель на старый блок памяти будет утерян. Чем это чревато, объяснять, думаю, не стоит.

Предупреждение анализатора: V801 Decreased performance. It is better to redefine the fifth function argument as a reference. Consider replacing 'const .. fmt' with 'const .. &fmt'. stelutils.cpp 742



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

Предупреждение анализатора: V803 Decreased performance. In case 'iterIn' is iterator it's more effective to use prefix form of decrement. Replace iterator-- with --iterator. meteorshowers.cpp 572



Здесь наблюдается интересный момент. Как известно, префиксные операции инкремента и декремента выполняются быстрее постфиксных. В цикле для перемещения итератора на следующую позицию используется префиксный инкремент, однако для изменения индекса и при выполнении операции декремента, используются их постфиксные формы. В любом случае, если есть возможность, лучше использовать префиксные формы операции инкремента и декремента. Анализатором было выявлено 16 участков подобного кода.


Заключение

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

P.S.: Также привожу список аккаунтов в других сетях:
twitter - https://twitter.com/Coder_CPP
reddit - https://www.reddit.com/user/Coder_CPP/
Medium - https://medium.com/@Coder_CPP


Это сообщение отредактировал(а) CoderCPP - 23.3.2015, 17:56