Geant4 project continues developing, so it's really interesting to recheck it with PVS-Studio static code analyzer. This time we'll do a check of version 10.2 (previously, we checked 10.0 beta-version) .
Geant4 toolkit is developed in CERN, for the simulation and exploration of particle behavior when passing through matter, using Monte-Carlo methods. Early versions of the project were written in Fortran, and starting with version 4, the project was fully translated into object-oriented language C++.
More details about this project can be found on the official site for the project: http://geant4.org.
This project was already checked a couple of times; you can find the results in other articles. The analysis of version 9.4 is described in the article "Copy-Paste and Muons", and the check of version 10.0-beta is described in the article "Going On with the Check of Geant4"
Since the last time we checked the project, Geant 4 was upgraded to version 10.02. PVS-Studio was also updated to version 6.05, so that was the version we used.
In the project I've encountered quite a number of errors, related to the usage of conditions and comparisons. Logical errors are usually done upon leaving the code for future development, or inaccurate modification, with the removal of previous parts of the code that contain branching statements. At the same time, simple typos and lack of reasoning the expressions may lead to errors or redundant code.
There was some zest in this check of Geant4, because as far as I understand, the development team already uses a static code analyzer, Coverity, regularly. I drew this conclusion by looking at various Release Notes, and comments in the code like this one:
// Private copy constructor and assigment operator - copying and
// assignment not allowed. Keeps Coverity happy.The Coverity analyzer is considered to be a leader in the market of code analyzers, so finding something after the Coverity analysis is already a great achievement. Nonetheless, PVS-Studio found plenty of interesting bugs, which also shows that it has become a powerful and mature product.
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
This is one of the most common errors when working with checks of several values of one variable using if. Of course, it could just be incorrect formatting, but in this example the analyzer most likely is pointing to a real bug.
As a result of the copying, the else word was left forgotten, which will lead in this case to the execution of excessive code. For example, the value will be zero, and we'll have the code executed from the corresponding block, but because of the error, the code from the else block after the comparison with one. To fix this issue, we should add the missing else before the condition 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
We can only assume what this code was meant for. It is very likely that this fragment is intended for catching and forming the error message, but in the cast of an incorrect condition, there will be no error message. It's unclear how the program will behave later on. Perhaps the handler will catch the bug in some different place, but there is a chance that the program will continue working without an error, but will output an incorrect result. It's quite hard to say exactly, what is the cause of this problem, as it can be both in one of the conditional expressions, as well as in the excessive else keyword. But judging by the formatting, we can safely assume that both conditional blocks are correct, and we should just remove else before the second conditional block.
Thanks to copy-paste this error was duplicated and was found in three more fragments:
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
We see errors related to pointer handling quite often in programs. In this case we have a situation where two objects are handled simultaneously, but only one is checked for correctness. This error may remain unnoticed for a long time, but if the pointer to the theShells turns out to be a null one, it will lead to undefined program behavior. To fix this, you need to change the condition as follows:
if ( theGammas != NULL && theShells != NULL) ....One more fragment where the check of the pointer is missing.
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
This fragment is similar to the previous one. But here's a pointer explicitly assigned with a zero value, and after that the variable is used for the initialization of other variables. The programmer may have intended to use the variable value from the first assignment, so the second is simply unnecessary. Perhaps, 0 was supposed to be assigned to a different variable. The true reasons for this assignment are known only by the developers of the project. In any case such initialization is not correct, and this code fragment is worth reviewing.
#define dependentAxis 1
#define allowByRegion 2
static enum xDataTOM_interpolationFlag
xDataTOM_interpolation_getFromString( .... ) {
....
if( flag | allowByRegion ) {....} // <=
if( flag | dependentAxis ) {....} // <=
....
}The analyzer issued a warning for two neighboring strings of a function. We have bitwise OR with a non-zero constant inside a condition. The result of such an expression will always be non-zero, which leads to incorrect logic in the program. Such errors often occur because of typos. Also in the condition, instead of the bitwise OR, another bitwise operation should be used. I suppose that in this case, the author meant to use bitwise AND, so it should look as follows:
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
In this code fragment we see a logic error that is in the redundant condition statment. One of the variants of what was meant to be here: during the verification against one, the variable was to be assigned with a different variable, whose name is also similar with sumnorm. But as in there were no such variables noticed in the checked part of the code, I will hazard a guess that this is just a redundant check. To fix this, let's simplify the condition in the following way:
if ( noSurfaces == 0 )
{
....
sumnorm=apprnorm;
}
else if ( noSurfaces != 1 ) { sumnorm = sumnorm.unit(); }Another suspicious fragment:
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
Here we also see a logic error. The code inside the function, depending on the value of historySwitch, changes the saveHistory flag, and carries out an operation with the file; the result of which is reported by the flag. But after all the operations, the variable saveHistory is just assigned with a value historySwitch. This is strange because the value in the condition was already set, and we have messed it up. Most likely it's a redundant assignment, and it should be removed.
There is a similar error in another fragment:
bool parse(....)
{
....
if( (word0=="line_pattern") ||
(word0=="line_pattern") ) { .... }
....
}V501 There are identical sub-expressions '(word0 == "line_pattern")' to the left and right of the '||' operator. style_parser 1172
Most often this occurs when testing multiple variables of the same type within the same condition, and using Copy-Paste for its composition.
The example has quite a small code fragment where you can clearly see the error. In this case it is just a typo and it is most likely caused by the code being copied. But this does not mean that it's easy to detect it doing simple check. This condition was taken from a long tree of various checks. The analyzer is especially useful in the detection of such constructions, and prevents errors during code refactoring.
Even if it's not an error, the code requires fixing, so that the double check doesn't confuse the person who will maintain this code.
Similar fragments were found in other parts of the project.
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
Sometimes during the process of refactoring, you may have fragments that remain unchanged. This is exactly what happened in this example. The old message was commented, the new one became the same as the additional check inside. To fix this you need to more carefully consider the correction of the code block, or just remove the extra check inside conditions.
Fragments with similar issues:
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
In the given block, the condition in the else section is redundant. The prerequisite for the entrance to the else block is already a false value of gflashSensitive variable, so it doesn't need to be checked once more.
Another similar fragment:
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
The same variable is checked in the inner condition block. This check will always generate a positive result because it was a condition for the entry to the inner condition block. As a result, the code will never be executed in the inner else block.
The same redundant check was found in several other fragments in the project. Oh, this Copy-Paste:
void G4XXXStoredViewer::DrawView() {
....
if (kernelVisitWasNeeded) {
DrawFromStore();
} else {
DrawFromStore();
}
....
}V523 The 'then' statement is equivalent to the 'else' statement. g4xxxstoredviewer.cc 85
The code inside the two branches is identical, which makes the condition useless, as the same code will be executed regardless of it. Analyzer messages of this kind might signal code that wasn't properly catered for, or typos when copying various constants or functions having similar names. In this case it's not clear what this block was made for, but it clearly needs to be reviewed and fixed.
There was another similar fragment:
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
This code fragment doesn't have an error, but it can be simplified in the following way:
if (!v || *v == fLastv) return;Several more similar fragments:
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
Instead of accessing the current object, a new temporary object is created and then immediately destroyed. As a result, the fields of the object will not be initialized. If you need to use field initialization outside the constructor, it's better to create a separate function and access it. But if you want to call the constructor, you should access the constructor using the word this. If you use C++11, the most graceful decision would be to use a delegate constructor. More details about these errors, and the ways to fix them, can be found in this book (see section 19, "How to properly call one constructor from another").
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
Here we have an error in an array initialization with the constants. As the result of the typo, a comma is missing. There are several troubles at the same time:
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
The major part of the function does the forming of a message to create an exception. But because of a missing throw, there will be an unused exception created. The program will continue working, which can lead to undefined behavior, or to incorrect evaluations.
The error was repeated in another parts of the project.
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
This error is caused by the mismatch of actual string length, and the argument that specifies the length inside the function. In this case an error occurred due to the formation of a particular indent created by spaces, at first glance it's hard to say how many of them are there. Perhaps this error wasn't taken into account, as it is still there from the last time we checked the project. This bug was included in the database of examples for V666 diagnostic.
Perhaps not all the listed errors are really dangerous, but a lot of minor bugs can lead to more serious consequences in the future. This is why you should regularly check your projects to detect errors during the early stages, before they lead to serious consequences. The analyzer is of great help in finding and fixing the trickiest bugs, and detecting hazardous places in the project before they turn into bugs. I suggest downloading and trying out PVS-Studio analyzer on your project: http://www.viva64.com/en/pvs-studio/download/.