Проект Geant4 продолжает развиваться, поэтому интересно вновь проверить его с помощью статического анализатора кода PVS-Studio. На этот раз проверке будет подвергнута версия 10.2 (предыдущая проверка относилась к версии 10.0-beta).
Инструментарий Geant4 разработан в CERN и предназначен для моделирования и исследования поведения частиц при прохождении через вещество, с использованием методов Монте-Карло. Ранние версии проекта была написаны на языке Fortran, а начиная с 4 версии проект был полностью переведён на объектно-ориентированный язык С++.
Более подробно о проекте можно узнать на официальном сайте проекта http://geant4.org.
Пару раз проект Geant4 уже проверялся, и о результатах рассказывают другие статьи. О проверке версии 9.4 можно прочитать в статье "Copy-Paste и мюоны", а о проверке версии 10.0-beta в статье "Продолжение проверки Geant4".
Со времени последней проверки была выпущена новая версия Geant4.10.2. PVS-Studio также обновился с того времени, и для проверки была использована версия анализатора 6.05.
В проекте мне попалось достаточно много ошибок, связанных с условиями и сравнениями. Логические ошибки чаще всего совершаются при оставлении кода на будущую доработку, или же неаккуратной модификации с удалением предыдущих, контролирующих переходы, частей кода. Но и простые опечатки и недостаточная продуманность выражений так же могут привести к ошибкам или избыточному коду.
Дополнительную пикантность очередной проверке проекта придаёт то, что проект Geant4, как я понимаю, регулярно проверяется статическим анализатором Coverity. Об этом свидетельствует множественные записи ReleaseNotes и комментарии в коде, вида:
// Private copy constructor and assigment operator - copying and
// assignment not allowed. Keeps Coverity happy.Анализатор Coverity считается лидером рынка, поэтому найти что-то после него является хорошим достижением. И тем не менее PVS-Studio нашел массу интересных ошибок, что говорит, что он тоже стал зрелым и мощным продуктом.
G4double G4EmBiasingManager::ApplySecondaryBiasing(....)
{
....
if(0 == nsplit) {
....
} if(1 == nsplit) { // <=
....
} else {
....
}
....
}V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. g4embiasingmanager.cc 299
Это одна из частых ошибок при работе с проверкой нескольких значений одной переменой с помощью if. Конечно это может быть просто неверное форматирование, но в этом примере анализатор скорее всего указывает именно на ошибку.
В результате копирования кода оказалось забыто слово else. Что в данном случае приведёт к выполнению лишнего кода. Например, значение будет равно нулю и будет выполнен код из советующего блока, но из-за ошибки также будет выполнен код блока else после проверки на равенство единице. Для исправления ошибки надо добавить пропущенное else перед условием if(1 == nsplit).
void G4GenericPolycone::Create( .... )
{
....
G4double rzArea = rz->Area();
if (rzArea < -kCarTolerance)
rz->ReverseOrder();
else if (rzArea < -kCarTolerance) // <=
{
....
G4Exception("G4GenericPolycone::Create()",
"GeomSolids0002",
FatalErrorInArgument, message);
}
....
}V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 102, 105. g4genericpolycone.cc 102
Могу лишь предполагать о назначении кода. Весьма вероятно, что этот фрагмент отвечает за перехват и формирование сообщения об ошибке, но в результате неверного условия сообщения об ошибке не будет. И неизвестно, как потом поведёт себя программа. Возможно, ошибку перехватит обработчик в другом месте, а может быть и такое, что программа продолжит выполнение без ошибки и выдаст некорректный результат работы. Точно сказать, в чём ошибка, достаточно сложно, так как она может быть как в одном из условных выражений, так и в лишнем else. Но судя по форматированию, можно смело предполагать, что оба блока условия верные и для исправления достаточно убрать else перед вторым условным блоком.
Благодаря копированию кода эта же ошибка была размножена и обнаруживается ещё в трёх местах проекта:
G4double * theShells;
G4double * theGammas;
void G4ParticleHPPhotonDist::InitAngular(....)
{
....
if ( theGammas != NULL )
{
for ( i = 0 ; i < nDiscrete ; i++ )
{
vct_gammas_par.push_back( theGammas[ i ] );
vct_shells_par.push_back( theShells[ i ] );
....
}
}
if ( theGammas == NULL ) theGammas = new G4double[nDiscrete2];
if ( theShells == NULL ) theShells = new G4double[nDiscrete2];
....
}V595 The 'theShells' pointer was utilized before it was verified against nullptr. Check lines: 147, 156. g4particlehpphotondist.cc 147
Ошибки при работе с указателями встречаются в программах достаточно часто. Здесь приведён случай, когда происходит одновременная работа с двумя объектами, а на корректность проверяется только один из них. Такая ошибка может долго оставаться незамеченной, но в случае, если указатель на объект theShells окажется нулевым, это приведет к неопределённому поведению программы. Для исправления ошибки требуется изменить условие следующим образом:
if ( theGammas != NULL && theShells != NULL) ....Ещё один фрагмент с отсутствием проверки указателя:
G4hhElastic::G4hhElastic(....)
: G4HadronElastic("HadrHadrElastic")
{
....
fTarget = target; // later vmg
fProjectile = projectile;
....
fTarget = G4Proton::Proton(); // later vmg
fProjectile = 0; // <=
fMassTarg = fTarget->GetPDGMass();
fMassProj = fProjectile->GetPDGMass(); // <=
....
}V522 Dereferencing of the null pointer 'fProjectile' might take place. g4hhelastic.cc 184
Этот фрагмент похож на предыдущий. Но здесь указателю явно присваивается нулевое значение, и после переменная используется для инициализации других переменных. Возможно, предполагалось использование значение переменой из первого присваивания, и тогда второе просто лишнее. Возможно, хотели присвоить 0 другой переменной. Истинные причины такого присваивания известны только разработчикам проекта. В любом случае подобная инициализация некорректна, и на такой фрагмент кода стоит обратить внимание.
#define dependentAxis 1
#define allowByRegion 2
static enum xDataTOM_interpolationFlag
xDataTOM_interpolation_getFromString( .... ) {
....
if( flag | allowByRegion ) {....} // <=
if( flag | dependentAxis ) {....} // <=
....
}Анализатор выдал предупреждение сразу на две соседних строки функции. Внутри условия происходит побитовое ИЛИ с ненулевой константой. Результатом такого выражения всегда будет ненулевое значение, что приводит к неправильной логике работы программы. Чаще всего подобные ошибки происходят в результате опечаток. И в условии вместо побитового ИЛИ должна быть использована другая побитовая операция. Могу предположить, что в данном случае имелось ввиду побитовое И. Исправленный код будет выглядеть так:
if( flag & allowByRegion ) {....}
if( flag & dependentAxis ) {....}G4ThreeVector G4GenericTrap::SurfaceNormal(....) const
{
....
if ( noSurfaces == 0 )
{
....
sumnorm=apprnorm;
}
else if ( noSurfaces == 1 ) { sumnorm = sumnorm; } // <=
else { sumnorm = sumnorm.unit(); }
....
}V570 The 'sumnorm' variable is assigned to itself. g4generictrap.cc 515
В этом фрагменте видна логическая ошибка, которая заключается в избыточной ветке условия. Один из вариантов, что хотел сделать программист: при проверке на равенство единице переменой должна была присваиваться другая переменная, название которой схоже с sumnorm. Но так как в проверяемой части кода не было замечено таких переменных, рискну предположить, что это просто лишняя проверка. И для исправления ошибки предлагаю упростить условие, следующим образом:
if ( noSurfaces == 0 )
{
....
sumnorm=apprnorm;
}
else if ( noSurfaces != 1 ) { sumnorm = sumnorm.unit(); }Ещё одно подозрительное место.
void G4UImanager::StoreHistory(G4bool historySwitch,....)
{
if(historySwitch)
{
if(saveHistory)
{ historyFile.close(); }
historyFile.open((char*)fileName);
saveHistory = true;
}
else
{
historyFile.close();
saveHistory = false;
}
saveHistory = historySwitch;
}V519 The 'saveHistory' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 541, 543. g4uimanager.cc 543
Тут так же видна логическая ошибка. Код внутри функции в зависимости от значения historySwitch изменяют значение флага saveHistory и проводят операцию с файлом, о результате которой и сообщает флаг. Но после выполнения всех операций к переменной saveHistory просто присваивается значение historySwitch. Это является странной операцией, так как значение внутри условия уже было установлено, и мы только его испортим. Скорее всего, последнее присваивание лишнее и его нужно удалить.
Похожая ошибка есть в другом месте:
bool parse(....)
{
....
if( (word0=="line_pattern") ||
(word0=="line_pattern") ) { .... }
....
}V501 There are identical sub-expressions '(word0 == "line_pattern")' to the left and to the right of the '||' operator. style_parser 1172
Чаще всего такая ошибка возникает при проверке множества однотипных переменных внутри одного условия и использования метода Copy-Paste для его формирования.
Пример содержит достаточно маленький фрагмент кода, где ошибка видна явно. В данном случае это просто опечатка и вызвана она скорее всего именно копированием кода. Но это не значит, что её просто обнаружить при обычной проверке. Данное условие было взято из длинного дерева разнообразных проверок. Именно такие конструкции хорошо помогает выявить анализатор, и предотвратить ошибки при рефакторинге кода.
Даже если это не ошибка, код всё равно требуется исправить, чтобы двойная проверка не смутила человека, который будет поддерживать этот код.
Похожие фрагменты были обнаружены и в других местах проекта.
G4ReactionProduct * G4ParticleHPLabAngularEnergy::Sample(....)
{
....
//if ( it == 0 || it == nEnergies-1 )
if ( it == 0 )
{
if(it==0) ....
....
}
....
}V571 Recurring check. The 'if (it == 0)' condition was already verified in line 123. g4particlehplabangularenergy.cc 125
Иногда в процессе рефакторинга кода остаются не до конца изменённые фрагменты. Так произошло и в примере. Старое условие было закомментировано, а новое стало точно соответствовать дополнительной проверке внутри. Для исправления ошибки необходимо лучше продумать исправления блока кода или просто убрать дополнительную проверку внутри условия.
Фрагменты с похожими недоработками:
void GFlashHitMaker::make(....)
{
....
if( gflashSensitive )
{
gflashSensitive->Hit(&theSpot);
}
else if ( (!gflashSensitive ) &&
( pSensitive ) &&
(....)
){....}
....
}V560 A part of conditional expression is always true: (!gflashSensitive). gflashhitmaker.cc 102
В приведённом блоке условие в разделе else является избыточным. Условием входа в блок else итак является ложное значение переменной gflashSensitive и проверять её во второй раз не требуется.
Ещё одно похожее место:
void UseWorkArea( T* newOffset )
{
....
if( offset && offset!=newOffset )
{
if( newOffset != offset ) {....}
else {....}
}
....
}V571 Recurring check. The 'newOffset != offset' condition was already verified in line 154. g4geomsplitter.hh 156
Одна и та же переменная повторно проверяется во внутреннем блоке условия. Эта проверка всегда будет выдавать положительный результат, так как это являлось условием входа во внутренний блок условия. И как следствие, код во внутреннем блоке else никогда не будет выполнятся.
В результате копирования кода эта же лишняя проверка присутствует в других местах проекта. Ох уж этот Copy-Paste:
void G4XXXStoredViewer::DrawView() {
....
if (kernelVisitWasNeeded) {
DrawFromStore();
} else {
DrawFromStore();
}
....
}V523 The 'then' statement is equivalent to the 'else' statement. g4xxxstoredviewer.cc 85
Код внутри двух веток условия идентичен, что делает само условие бесполезным, так как один и тот же код будет выполнен независимо от него. Подобные сообщения анализатора могут сигнализировать о недоработанном коде или об опечатках при копировании различных констант или близких по названию функций. В данном случае неизвестно, для чего был сделан этот блок, но его явно требуется доработать.
Подобный фрагмент встречается ещё в одном месте проекта.
Void G4VTwistSurface::CurrentStatus::ResetfDone(....)
{
if (validate == fLastValidate && p && *p == fLastp)
{
if (!v || (v && *v == fLastv)) return;
}
....
}V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!v' and 'v'. g4vtwistsurface.cc 1198
Данный фрагмент не является ошибкой. Но условие можно упростить следующим образом:
if (!v || *v == fLastv) return;Похожие фрагменты есть и в других местах.
class G4PhysicsModelCatalog
{
private:
....
G4PhysicsModelCatalog();
....
static modelCatalog* catalog;
....
};
G4PhysicsModelCatalog::G4PhysicsModelCatalog()
{ if(!catalog) {
static modelCatalog catal;
catalog = &catal;
}
}
G4int G4PhysicsModelCatalog::Register(const G4String& name)
{
G4PhysicsModelCatalog();
....
}V603 The object was created but it is not being used. If you wish to call constructor, 'this->G4PhysicsModelCatalog::G4PhysicsModelCatalog(....)' should be used. g4physicsmodelcatalog.cc 51
Вместо обращения к текущему объекту происходит создание нового временного объекта, который будет сразу уничтожен. В результате поля объекта не будут инициализированы. Когда требуется использовать инициализацию полей вне конструктора, то лучше для этого создать отдельную функцию и обращаться к ней. Но если требуется вызвать именно конструктор, необходимо обращается к конструктору с помощью слова this. В случае, если используется C++11, то наиболее красивым решением будет использование делегирующего конструктора. Подробнее подобные ошибки и способ их исправления рассматривается в этой книге (см. раздел 19 "Как правильно вызвать один конструктор из другого").
static const G4String name[numberOfMolecula] = {
....
"(CH_3)_2S", "N_2O",
"C_5H_10O" "C_8H_6", "(CH_2)_N",
....
};V653 A suspicious string consisting of two parts is used for array initialization. It is possible that a comma is missing. Consider inspecting this literal: "C_5H_10O" "C_8H_6". g4hparametrisedlossmodel.cc 324
Здесь допущена ошибка инициализации массива с помощью констант. В результате опечатки была пропущена запятая. Здесь сразу несколько бед:
class G4HadronicException : public std::exception {....}
void G4CrossSectionDataStore::ActivateFastPath( ....)
{
....
if ( requests.insert( { key , min_cutoff } ).second ) {
....
G4HadronicException(__FILE__,__LINE__,msg.str());
}
}V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw G4HadronicException(FOO); g4crosssectiondatastore.cc 542
Большая часть кода функции занимается формированием сообщения для создания исключения. Но из-за пропущенного throw будет создано неиспользуемое исключение. И программа продолжит работу, что может привести к неопределённому поведению программы или к некорректным расчетам.
Ошибка была повторяется в других местах проекта.
bool G4GMocrenIO::storeData2() {
....
ofile.write("GRAPE ", 8);
....
}V666 Consider inspecting second argument of the function 'write'. It is possible that the value does not correspond with the length of a string which was passed with the first argument. g4gmocrenio.cc 1351
Приведённая ошибка связана с несоответствием фактической длинны строки и аргумента, который задаёт длину внутри функции. В данном случае ошибка возникла из-за формирования определённого отступа с помощью пробелов, и на первый взгляд не видно сколько их там на самом деле. Возможно поэтому этой ошибки не было уделено должное внимание, и она находится в проекте ещё со времени первой проверки. Эта ошибка была включена в базу примеров для диагностики V666.
Возможно, не все приведённые ошибки являются опасными, но многие на первый взгляд незначительные ошибки могут в будущем привести к более серьёзным последствиям. Поэтому проекты требуется проверять регулярно, чтобы выявлять ошибки на ранних стадиях до того, как они приведут к серьёзным последствиям. Анализатор поможет не только найти и исправить самые заковыристые ошибки, но и обнаружить опасные места в проекте прежде, чем они станут ошибками. Предлагаю скачать и попробовать анализатор PVS-Studio в своём проекте: http://www.viva64.com/ru/pvs-studio/download/.