Краткое содержание предыдущей статьи
В статье «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. Там же приводятся предупреждения, относящиеся к микрооптимизациям.
Заключение
На тему бессмысленности проверки старого проекта. Вот видите, и я косанул. :)
Хороший повод меня потроллить.
This entry passed through the Full-Text RSS service — if this is your content and you're reading it on someone else's site, please read the FAQ at fivefilters.org/content-only/faq.php#publishers.
Комментариев нет:
Отправить комментарий