Topic: Hidden bugs due to C-style casts
I've been working through a few (thousand) warnings from CppCheck recently, the most common warnings can be summed up as
C-style casts - We should always use static_cast<> for converting between classes. C-style casts will automatically do a reinterpret_cast<> when static_cast<> doesn't work, which will definitely lead to broken code (see below)
Constructors doesn't initialize all members. This is an obvious advantage, even if they just set values to zero. Use the initializer list if possible.
Local scopes for variables. Especially for-loops and such, they should declare their variables locally. I've fixed several bugs caused by for-loop-counters are accessed outside their loops. Local scopes help to break down code
W.r.t to local scopes i mean;
// Prefer local scopes for loops;
for (int i = 1; i <= 3; i++) {
int n = ...
}
// as opposed to
int i, n;
for (i = 1; i <= 3; i++) {
n = ...
}at least w.r.t simple integers, pointers and such. For FloatArrays and such, there are good reasons to define them outside the loops.
For C-style casts, this hides potential errors.
int
PatchIntegrationRule :: SetUpPointsOnTriangle(int nPoints, MaterialMode mode)
{
numberOfIntegrationPoints = GaussIntegrationRule :: SetUpPointsOnTriangle(nPoints, mode);
firstLocalStrainIndx = 1;
lastLocalStrainIndx = 3;
// convert patch coordinates into element based, update weights accordingly
for ( int j = 0; j < numberOfIntegrationPoints; j++ ) {
GaussPoint *gp = this->gaussPointArray[ j ];
patch->convertGPIntoParental(gp); // convert coordinates into parental
Element *elg = patch->giveParent();
double parentArea = elg->computeArea();
Triangle *tr = ( Triangle * ) patch; ///@todo This must be a serious bug. This will do a reinterpret_cast from Patch to Triangle, which makes no sense.
gp->setWeight(8.0 * gp->giveWeight() * tr->getArea() / parentArea); // update integration weight
}
return numberOfIntegrationPoints;
}Triangle and Patch are not compatible. I'm not sure if this was meant to cast to TrianglePatch. This cast will become an reinterpret_cast<> which will only produce a bunch of nonsense results.