Краткое содержание предыдущей статьи
В статье «Copy-Paste и мюоны» рассказывается о пользе использования методологии статического анализа и возможностях PVS-Studio. Был проверен бородатый проект Geant4 версии 4.9.4. Было найдено множество подозрительных фрагментов кода, которые и были описаны статье.
Geant4 это программа для моделирования прохождения элементарных частиц через вещество с использованием методов Монте-Карло. Разработана в CERN на объектно-ориентированном языке программирования С++. Является дальнейшим развитием предыдущих версий GEANT, существенно переработанным и дополненным.
Сайт проекта: http://geant4.org.
В предыдущей статье было описано 16 крайне подозрительных фрагментов кода. Новая проверка выявила только 10 из них. Остальные видимо или исправлены, или этого кода больше не существует. Я не буду вновь описывать в новой статье эти фрагменты кода. Их можно увидеть в предыдущей статье. В ней помечено, исчезло предупреждение анализатора или нет в новой версии.
Прошу прощения, что получился такой странный формат исследования. Но надеюсь, это не помешает исправить ряд недочетов в проекте Geant4 и привлечь внимание людей к инструменту PVS-Studio.
Если я не ошибаюсь, предыдущая версия библиотеки относится к 2011 году. За это время многое изменилось, и не удивительно, что найдены некоторые новые странные места в коде. Давайте посмотрим, что есть новенького или то, что я не заметил ранее.
Новые подозрительные фрагменты кода
Полный список подозрительных мест, на которые я обратил внимание, находится в файле geant4_new.txt. Хочу подчеркнуть, что не стоит полагаться исключительно на этот список. Намного лучше, будет проверить проект самостоятельно и проанализировать все предупреждения. Мы готовы предоставить для этого на некоторое время ключ разработчикам Geant4: форма обратной связи.
Одинаковые функции
G4double G4CsvAnalysisManager::GetH2Xmax(G4int /*id*/) const { ExceptionForHistograms("GetH2Xmin"); return 0; } G4double G4CsvAnalysisManager::GetH2Xmax(G4int /*id*/) const { ExceptionForHistograms("GetH2Xmin"); return 0; }Предупреждение PVS-Studio: V524 It is odd that the body of ‘GetH2Xmax’ function is fully equivalent to the body of ‘GetH2Xmin’ function. _G4analysis-archive g4csvanalysismanager.cc 933
Функция GetH2Xmax(), по всей видимости, должна вызывать функция ExceptionForHistograms() с другим параметром:
ExceptionForHistograms("GetH2Xmax");Ошибка не выглядит серьезной. Как я понимаю, это обработка исключительной ситуации. Тем не менее, решил упомянуть об этой Copy-Paste ошибке.
Энергия равна нулю
Функции CalculateTotalEnergy () суммирует значения в переменной ‘Etot’. Неожиданно, эта переменная обнуляется. Это очень подозрительно.
G4double G4RKFieldIntegrator::CalculateTotalEnergy(const G4KineticTrackVector& Barions) { G4double Etot = 0; .... for(G4int c2 = c1 + 1; c2 < nBarion; c2++) { .... // Esk2 Etot += t1*std::pow(Alpha/pi, 3/2)* std::exp(-Alpha*r12*r12); // Eyuk Etot += ....; // Ecoul Etot += 1.44*p1->GetDefinition()->GetPDGCharge()* p2->GetDefinition()->GetPDGCharge()/r12* Erf(std::sqrt(Alpha)*r12); // Epaul Etot = 0; .... } .... }Предупреждение PVS-Studio: V519 The ‘Etot’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 80, 83. _G4processes-archive g4rkfieldintegrator.cc 83
Подозрительная логика
G4double G4EmBiasingManager::ApplySecondaryBiasing(....) { .... if(0 == nsplit) { if(safety > fSafetyMin) .... } if(1 == nsplit) { weight = ApplyRussianRoulette(vd, index); } else { G4double tmpEnergy = pPartChange->GetProposedKineticEnergy(); G4ThreeVector tmpMomDir = .... weight = ApplySplitting(vd, track, currentModel, index, tcut); pPartChange->SetProposedKineticEnergy(tmpEnergy); pPartChange->ProposeMomentumDirection(tmpMomDir); } .... }Предупреждение PVS-Studio: V646 Consider inspecting the application’s logic. It’s possible that ‘else’ keyword is missing. _G4processes-archive g4embiasingmanager.cc 299
Код оформлен так, как будто используется конструкция «else if». Но это не так. Если оформить код правильно, то получим вот это:
if(0 == nsplit) { if(safety > fSafetyMin) .... } if(1 == nsplit) { weight = ApplyRussianRoulette(vd, index); } else { G4double tmpEnergy = pPartChange->GetProposedKineticEnergy(); G4ThreeVector tmpMomDir = .... weight = ApplySplitting(vd, track, currentModel, index, tcut); pPartChange->SetProposedKineticEnergy(tmpEnergy); pPartChange->ProposeMomentumDirection(tmpMomDir); }Обратите внимание, что блок, относящийся к ветке ‘else’ выполняется всегда, если «1 != nsplit». Есть подозрение, что задумывалась иная логика работы.
Аналогичную ситуацию можно увидеть здесь:
V646 Consider inspecting the application’s logic. It’s possible that ‘else’ keyword is missing. _G4processes-archive g4embiasingmanager.cc 347
Недописанный код?
void G4MolecularDecayTable::AddExcitedState(const G4String& label) { channelsMap::iterator channelsIter = fDecayChannelsMap.find(label); if(channelsIter != fDecayChannelsMap.end()) { G4String errMsg = "Excited state" + label + " already registered in the decay table."; G4Exception("G4MolecularDecayTable::AddExcitedState", "G4MolecularDecayTable003", FatalErrorInArgument, errMsg); return; } fDecayChannelsMap[label] ; }Предупреждение PVS-Studio: V607 Ownerless expression ‘fDecayChannelsMap[label]’. _G4processes-archive g4moleculardecaytable.cc 140
Конец функции выглядит очень подозрительно:
fDecayChannelsMap[label] ;Что это? Здесь чего-то не хватает? Что хотели сделать с ячейкой массива?
Кинематика
Следующий пример достаточно длинный. К сожалению, мне трудно сократить его ещё больше. Посмотрите, какие значения может принимать переменная ‘id’.
void G4QMDCollision::CalKinematicsOfBinaryCollisions( G4double dt) { .... G4int id = 0; .... if ( secs ) { .... id++; .... } if ( std::abs ( eini - efin ) < fepse*10 ) .... else { .... for ( G4int i0i = 0 ; i0i < id-1 ; i0i++ ) { theSystem->DeleteParticipant( i0i+n0 ); } .... } .... }Предупреждение PVS-Studio: V621 Consider inspecting the ‘for’ operator. It’s possible that the loop will be executed incorrectly or won’t be executed at all. _G4processes-archive g4qmdcollision.cc 228
Если условие «if ( secs )» не выполнится, то переменная ‘id’ останется равна нулю. Тогда, может возникнуть цикл вида:
for ( G4int i0i = 0 ; i0i < -1 ; i0i++ )Это будет очень странный цикл. Думаю, стоит проверить логику работы функции CalKinematicsOfBinaryCollisions().
Прочее
Есть ещё ряд предупреждений, которые я не описывал в прошлой статье. Не буду и в этой. Для примера, покажу один случай:
class G4HadronicException : public std::exception { .... }; inline G4double G4GeneralPhaseSpaceDecay::Pmx( G4double e, G4double p1, G4double p2) { if (e-p1-p2 < 0 ) { G4HadronicException(__FILE__, __LINE__, "G4GeneralPhaseSpaceDecay::Pmx " "energy in cms > mass1+mass2"); } .... }Предупреждение PVS-Studio. V596 The object was created but it is not being used. The ‘throw’ keyword could be missing: throw G4HadronicException(FOO); _G4processes-archive g4generalphasespacedecay.hh 116
Есть несколько ошибок, когда забыт оператор ‘throw’. В результате, создаётся и тут же уничтожается объект, имеющий тип ‘G4HadronicException’. И дальше программа продолжает работать с некорректными данными.
Вот такие опечатки, неописанные в статье, можно найти в файле geant4_new.txt. Там же приводятся предупреждения, относящиеся к микрооптимизациям.
Заключение
На тему бессмысленности проверки старого проекта. Вот видите, и я косанул. 🙂
Хороший повод меня потроллить.
ссылка на оригинал статьи http://habrahabr.ru/company/pvs-studio/blog/202158/
Добавить комментарий