Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
Webinar: Evaluation - 05.12
This post continues the series of articles, which can well be called "horrors for developers". This time it will also touch upon a typical pattern of typos related to the usage of numbers 0, 1, 2. The language you're writing in doesn't really matter: it can be C, C++, C#, or Java. If you're using constants 0, 1, 2 or variables' names contain these numbers, most likely, Freddy will come to visit you at night. Go on, read and don't say we didn't warn you.
I continue the series of articles on the patterns noticed of how people make mistakes. Previous posts:
This time it wasn't me who took note of the pattern, but my colleague Svyatoslav Razmyslov. He noticed that in his articles he was constantly describing problems involving variables with numbers 1 and 2 in their names. Svyatoslav invited me to explore this issue in more detail. Eventually, efforts made ended up very fruitful. It emerged that in our error collection there are a lot of code fragments that are erroneous because of the fact that people got confused in 0, 1, 2 indexes or variables names, containing such numbers. A new interesting pattern has been revealed, which will be discussed below. I am grateful to Svyatoslav for a hint to look into this topic and, therefore, I dedicate this article to him.
Svyatoslav Razmyslov, manager, attentive bug hunter and just a talented person.
What is the purpose of this article? To show how easy it is for all of us to make mistakes and make typos. Forewarned developers - more attentive developers. Especially during code reviews when they focus on these ill-fated 0, 1, 2. Developers will also be able to appreciate the contribution of static code analyzers that help to spot such errors. It's not about advertising PVS-Studio (well, to some extent, it is :). Until now, many developers consider static analysis superfluous, preferring to focus on their own accuracy and code reviews. Unfortunately, attempts to write clean code are laudable but not enough. This article will once again convincingly demonstrate this.
No one is immune to errors. Below you will see epic blunders in even such well-known projects as Qt, Clang, Hive, LibreOffice, Linux Kernel, .NET Compiler Platform, XNU kernel, Mozilla Firefox. By the way, these are not some exotic rare mistakes, but the most common ones. Still not convincing enough? Then let's get going!
"Talk is cheap. Show me bugs!"
(c) remade quote by Linus Torvalds.
Usually in our articles we cite warnings that helped to find certain errors. This time I'll omit these warnings, as even without them errors will still be obvious and clear. Even though these bugs leap out in a short code fragment, they are great at hiding in projects' code.
Let's start with confusions with numerical literals, used for arrays indexing. Despite the banality of these errors, they are many and they can be found in projects that are much greater than students' laboratory researches.
XNU kernel project, C
uint32_t
gss_krb5_3des_unwrap_mbuf(....)
{
....
for (cflag = 1; cflag >= 0; cflag--) {
*minor = gss_krb5_3des_token_get(
ctx, &itoken, wrap, &hash, &offset, &length, reverse);
if (*minor == 0)
break;
wrap.Seal_Alg[0] = 0xff;
wrap.Seal_Alg[0] = 0xff;
}
....
}The line was copied, but the index stayed the same. Most likely, the code here is supposed to be as follows:
wrap.Seal_Alg[0] = 0xff;
wrap.Seal_Alg[1] = 0xff;LibreOffice project, C++
Sequence< OUString > FirebirdDriver::
getSupportedServiceNames_Static() throw (RuntimeException)
{
Sequence< OUString > aSNS( 2 );
aSNS[0] = "com.sun.star.sdbc.Driver";
aSNS[0] = "com.sun.star.sdbcx.Driver";
return aSNS;
}As in the previous case, the authors copied the line, but forgot to change 0 for 1. Only fixed the string literal.
One might ask the philosophical question - how can you make such an error in a four-line function? You can and that's it. That's what programming is like.
Quake-III-Arena project, C
int VL_FindAdjacentSurface(....)
{
....
if (fabs(dir[0]) > test->radius ||
fabs(dir[1]) > test->radius ||
fabs(dir[1]) > test->radius)
{
....
}The developer forgot to change dir[1] for dir[2] in the copied line. As a result - the value on Z axis is out of control.
OpenCOLLADA project, C++
struct short2
{
short values[2];
short2(short s1, short s2)
{
values[0] = s1;
values[2] = s2;
}
....
};Yes, even in such a short constructor one can find a way to be out of array bounds during its initialization.
Godot Engine, C++
Array PhysicsDirectSpaceState::_cast_motion(....)
{
....
Array ret(true);
ret.resize(2);
ret[0]=closest_safe;
ret[0]=closest_unsafe;
return ret;
}No comment is needed.
Asterisk, C
static void sip_threadinfo_destructor(void *obj)
{
struct sip_threadinfo *th = obj;
struct tcptls_packet *packet;
if (th->alert_pipe[1] > -1) { // <=
close(th->alert_pipe[0]);
}
if (th->alert_pipe[1] > -1) {
close(th->alert_pipe[1]);
}
th->alert_pipe[0] = th->alert_pipe[1] = -1;
....
}When writing similar blocks, an error is usually in the last one. All above cases were like this, except for the last one. Here the typo is in an unusual place, namely, in the first block. It's hard to say why it happened so. I'll just leave the picture of a unicorn shrugging his shoulders:
Open CASCADE Technology, C++
inline void Prepend(const Standard_Integer theIndex)
{
if (myIndex[1] >= 0)
Standard_OutOfRange::Raise ("BRepMesh_PairOfIndex....");
myIndex[1] = myIndex[0];
myIndex[1] = theIndex;
}Different values are copied twice in the same array slot. Obviously, it's an error. The project code is unfamiliar to me, so it's not clear how to fix this bug. So I just looked at how the developers fixed the code after our team pointed out this error to them. Here is the correct version:
myIndex[1] = myIndex[0];
myIndex[0] = theIndex;Trans-Proteomic Pipeline, C++
void ASAPRatio_getProDataStrct(proDataStrct *data,
char **pepBofFiles)
{
....
if (data->indx == -1) {
data->ratio[0] = -2.;
data->ratio[0] = 0.; // <=
data->inv_ratio[0] = -2.;
data->inv_ratio[1] = 0.;
return;
}
....
}I'm concerned that such errors take place in research packages. Trans-Proteomic Pipeline is designed to handle the tasks in biology. One might make a real mess of things and screw up the entire research. We found many intriguing things in this package: check in 2012, check in 2013. Perhaps, we should take another look at this project.
ITK project, C++
Here is another project for medical research: Medicine Insight Segmentation and Registration Toolkit (ITK). The project is different, and the bugs are the same.
template< typename TCoordRepType >
void
VoronoiDiagram2D< TCoordRepType >::SetOrigin(PointType vorsize)
{
m_VoronoiBoundaryOrigin[0] = vorsize[0];
m_VoronoiBoundaryOrigin[0] = vorsize[1];
}ITK project, C++
int itkPointSetToSpatialObjectDemonsRegistrationTest(....)
{
....
// Set its position
EllipseType::TransformType::OffsetType offset;
offset[0]=50;
offset[1]=50;
offset[1]=50;
....
}Copy-Paste at its finest.
ReactOS project, C++
HPALETTE CardWindow::CreateCardPalette()
{
....
//include button text colours
cols[0] = RGB(0, 0, 0);
cols[1] = RGB(255, 255, 255);
//include the base background colour
cols[1] = crBackgnd;
//include the standard button colours...
cols[3] = CardButton::GetHighlight(crBackgnd);
cols[4] = CardButton::GetShadow(crBackgnd);
cols[5] = CardButton::GetFace(crBackgnd);
....
}Apparently, the crBackgnd constant had to be written in the cols[2] slot.
Coin3D project, C++
SoVRMLInline::GLRender(SoGLRenderAction * action)
{
....
if ((size[0] >= 0.0f && size[1] >= 0.0f && size[1] >= 0.0f) &&
((vis == ALWAYS) ||
(vis == UNTIL_LOADED && child == NULL))) {
....
}The size[1] array element is checked twice, whereas the size[2] element isn't checked at all. That's how strange artifacts appear in the images.
OpenCV project, C++
bool Jpeg2KDecoder::readHeader()
{
....
cmptlut[0] = ....
cmptlut[1] = ....
cmptlut[2] = ....
if( cmptlut[0] < 0 || cmptlut[1] < 0 || cmptlut[0] < 0 )
result = false;
....
}My gut is telling me that the cmptlut[0] < 0 expression was copied twice, but 0 was changed just once.
Visualization Toolkit (VTK) project, C++
void vtkImageStencilRaster::PrepareForNewData(....)
{
....
if (allocateExtent &&
allocateExtent[1] >= allocateExtent[1])
....
}In this case and later I won't comment many similar errors. Why comment? The main thing when looking through such code fragments is to become aware of the fact that even if the error is simple it doesn't mean a developer will definitely notice it.
Visualization Toolkit (VTK) project, C++
template <class iterT>
void vtkDataSetAttributesCopyValues(....)
{
....
inZPtr +=
(outExt[0] - outExt[0])*inIncs[0] * data_type_size +
(outExt[2] - outExt[2])*inIncs[1] * data_type_size +
(outExt[4] - outExt[4])*inIncs[2] * data_type_size;
....
}Here the programmer was clearly in a hurry and wrote the code very quickly. It's hard to explain how he made a mistake three times. The elements of the array are subtracted from themselves. The result is that this code equals the following:
inZPtr +=
(0)*inIncs[0] * data_type_size +
(0)*inIncs[1] * data_type_size +
(0)*inIncs[2] * data_type_size;However, this code can be shortened even more:
inZPtr += 0;Just great. There's a long, serious-looking expression in the code that doesn't really do anything. I just love such cases.
Visualization Toolkit (VTK) project, C++
A similar case of hasty coding.
void vtkPiecewiseControlPointsItem::SetControlPoint(
vtkIdType index, double* newPos)
{
double oldPos[4];
this->PiecewiseFunction->GetNodeValue(index, oldPos);
if (newPos[0] != oldPos[0] || newPos[1] != oldPos[1] ||
newPos[2] != oldPos[2] || newPos[2] != oldPos[2])
{
this->PiecewiseFunction->SetNodeValue(index, newPos);
}
}The newPos[2] != oldPos[2] comparison repeats twice.
ADAPTIVE Communication Environment (ACE), C++
bool URL_Base::strip_scheme (ACE_CString& url_string)
{
....
ACE_CString::size_type pos = url_string.find (':');
if (pos > 0 &&
url_string[pos+1] == '/' &&
url_string[pos+1] == '/')
{
....
// skip '<protocol>://'
url_string = url_string.substr (pos+3);
}
....
}The condition should check that there are two slashes after the colon. In other words, we look for the substring "://". Due to a typo, the check gets blinded and considers any character as a second slash.
IPP Samples, C++
void MeBase::MakeVlcTableDecision()
{
....
Ipp32s BestMV =
IPP_MIN(IPP_MIN(m_cur.MvRate[0],m_cur.MvRate[1]),
IPP_MIN(m_cur.MvRate[2],m_cur.MvRate[3]));
Ipp32s BestAC =
IPP_MIN(IPP_MIN(m_cur.AcRate[0],m_cur.AcRate[1]),
IPP_MIN(m_cur.AcRate[2],m_cur.AcRate[2]));
....
}The typo lies here in the macro arguments:
IPP_MIN(m_cur.AcRate[2],m_cur.AcRate[2])As a result, the minimum value is chosen from two equal ones. In fact, the following should be written:
IPP_MIN(m_cur.AcRate[2],m_cur.AcRate[3])By the way, this code might demonstrate the benefit of the standard library. If we write in the following way:
Ipp32s BestMV = std::min_element(begin(m_cur.MvRate), end(m_cur.MvRate));
Ipp32s BestAC = std::min_element(begin(m_cur.AcRate), end(m_cur.AcRate));The code will be shorter and less prone to errors. Actually, the less of the same-type code, the more likely it is to be written correctly.
Audacity, C++
sampleCount VoiceKey::OnBackward (....) {
....
int atrend = sgn(buffer[samplesleft - 2]-
buffer[samplesleft - 1]);
int ztrend = sgn(buffer[samplesleft - WindowSizeInt-2]-
buffer[samplesleft - WindowSizeInt-2]);
....
}Correct expression:
int ztrend = sgn(buffer[samplesleft - WindowSizeInt-2]-
buffer[samplesleft - WindowSizeInt-1]);PDFium, C++
void sycc420_to_rgb(opj_image_t* img) {
....
opj_image_data_free(img->comps[0].data);
opj_image_data_free(img->comps[1].data);
opj_image_data_free(img->comps[2].data);
img->comps[0].data = d0;
img->comps[1].data = d1;
img->comps[2].data = d2;
img->comps[1].w = yw; // 1
img->comps[1].h = yh; // 1
img->comps[2].w = yw; // 1
img->comps[2].h = yh; // 1
img->comps[1].w = yw; // 2
img->comps[1].h = yh; // 2
img->comps[2].w = yw; // 2
img->comps[2].h = yh; // 2
img->comps[1].dx = img->comps[0].dx;
img->comps[2].dx = img->comps[0].dx;
img->comps[1].dy = img->comps[0].dy;
img->comps[2].dy = img->comps[0].dy;
}Some actions aimed on initializing the structure repeat. Lines with the comment //2 can be removed without changing anything. I doubted about adding this code fragment in the article. It's not exactly an error, and not quite with indexes. Nevertheless, this redundant code has probably appeared here right because of the fact that the programmer got confused in all these class members and 1, 2 indexes. So I think this piece of code is great to demonstrate how easy it is to get confused in numbers.
CMake project, C
The code next up isn't written by CMake developers, but borrowed. As the comment says at the beginning of the file, the utf8_encode function was written by Tim Kientzle back in 2007. Since then, this function roams from project to project and can be met in many places. I didn't dig into the initial source, as it's not the matter of importance. Once the CMake project includes this code, the error applies to CMake as well.
static char *
utf8_encode(const wchar_t *wval)
{
....
p[0] = 0xfc | ((wc >> 30) & 0x01);
p[1] = 0x80 | ((wc >> 24) & 0x3f);
p[1] = 0x80 | ((wc >> 18) & 0x3f);
p[2] = 0x80 | ((wc >> 12) & 0x3f);
p[3] = 0x80 | ((wc >> 6) & 0x3f);
p[4] = 0x80 | (wc & 0x3f);
p += 6;
....
}As you can see, there is some confusion with the indexes. The value is written twice in the p[1] array element. If you look at the adjacent code, it becomes clear that the correct code should be this:
p[0] = 0xfc | ((wc >> 30) & 0x01);
p[1] = 0x80 | ((wc >> 24) & 0x3f);
p[2] = 0x80 | ((wc >> 18) & 0x3f);
p[3] = 0x80 | ((wc >> 12) & 0x3f);
p[4] = 0x80 | ((wc >> 6) & 0x3f);
p[5] = 0x80 | (wc & 0x3f);
p += 6;Note
Please, note that all errors in this section relate to the code in C and C++. There is no code in C# or Java!
It's very interesting, I didn't expect this. In my opinion, the typos considered don't depend on the language. In the sections below, there will be errors in code, written in other languages. I think it's just a coincidence. The PVS-Studio analyzer has started to support the C#/Java languages much later than C/C++, and we just didn't have enough time to collect the examples of the above errors types.
However, this conclusion is still interesting. Apparently, C and C++ programmers are more inclined to use numbers 0, 1, 2 when working with arrays :).
This will be the largest section. It is very easy for people to get confused in names such as a1 and a2. You might think: "How could you ever get confused here"? You can. And very easily. Now the reader will be able to see it.
Hive project, Java
@Override
public List<ServiceInstance> getAllInstancesOrdered() {
List<ServiceInstance> list = new LinkedList<>();
list.addAll(instances.values());
Collections.sort(list, new Comparator<ServiceInstance>() {
@Override
public int compare(ServiceInstance o1, ServiceInstance o2) {
return o2.getWorkerIdentity().compareTo(o2.getWorkerIdentity());
}
});
return list;
}The comparison function compare receives two objects: o1 and o2. But due to the typo only o2 is used.
Interestingly, this error has made its way to another function because of Copy-Paste:
@Override
public List<ServiceInstance> getAllInstancesOrdered() {
List<ServiceInstance> list = new LinkedList<>();
readLock.lock();
try {
list.addAll(instances.values());
} finally {
readLock.unlock();
}
Collections.sort(list, new Comparator<ServiceInstance>() {
@Override
public int compare(ServiceInstance o1, ServiceInstance o2) {
return o2.getWorkerIdentity().compareTo(o2.getWorkerIdentity());
}
});
return list;
}
Infer.NET project, C#
private void MergeParallelTransitions()
{
....
if (double.IsInfinity(transition1.Weight.Value) &&
double.IsInfinity(transition1.Weight.Value))
....
}Doom 3 project, C++
uint AltOp::fixedLength()
{
uint l1 = exp1->fixedLength();
uint l2 = exp1->fixedLength();
if (l1 != l2 || l1 == ~0u)
return ~0;
return l1;
}If you didn't notice the typo, look at the line, where the l2 variable is initialized. exp2 had to be used.
Source Engine SDK project, C++
void GetFPSColor( int nFps, unsigned char ucColor[3] )
{
....
int nFPSThreshold1 = 20;
int nFPSThreshold2 = 15;
if (IsPC() &&
g_pMaterialSystemHardwareConfig->GetDXSupportLevel() >= 95)
{
nFPSThreshold1 = 60;
nFPSThreshold1 = 50;
}
....
}The correct version:
nFPSThreshold1 = 60;
nFPSThreshold2 = 50;Linux Kernel project, C
By the way, in addition to variable names, typos can be in macros' names. Here are a few examples.
int private_ioctl(struct vnt_private *pDevice, struct ifreq *rq)
{
....
if (sStartAPCmd.byBasicRate & BIT3) {
pMgmt->abyIBSSSuppRates[2] |= BIT7;
pMgmt->abyIBSSSuppRates[3] |= BIT7;
pMgmt->abyIBSSSuppRates[4] |= BIT7;
pMgmt->abyIBSSSuppRates[5] |= BIT7;
} else if (sStartAPCmd.byBasicRate & BIT2) {
pMgmt->abyIBSSSuppRates[2] |= BIT7;
pMgmt->abyIBSSSuppRates[3] |= BIT7;
pMgmt->abyIBSSSuppRates[4] |= BIT7;
} else if (sStartAPCmd.byBasicRate & BIT1) { // <=
pMgmt->abyIBSSSuppRates[2] |= BIT7;
pMgmt->abyIBSSSuppRates[3] |= BIT7;
} else if (sStartAPCmd.byBasicRate & BIT1) { // <=
pMgmt->abyIBSSSuppRates[2] |= BIT7;
} else {
/* default 1,2M */
pMgmt->abyIBSSSuppRates[2] |= BIT7;
pMgmt->abyIBSSSuppRates[3] |= BIT7;
}
....
}As you can see, the mask with the BIT1 name is used twice, which makes the second check pointless. The body of the second conditional operator marked by the comment will never execute.
CMaNGOS project, C++
void AttackedBy(Unit* pAttacker) override
{
....
DoScriptText(urand(0, 1) ?
SAY_BELNISTRASZ_AGGRO_1 :
SAY_BELNISTRASZ_AGGRO_1,
m_creature, pAttacker);
....
}The project was intended to include random behavior, but the same constant SAY_BELNISTRASZ_AGGRO_1 is chosen every time.
Vangers project: One For The Road, C++
const char* iGetJoyBtnNameText(int vkey,int lang)
{
....
if (vkey >= VK_STICK_SWITCH_1 && vkey <= VK_STICK_SWITCH_9)
{
ret = (lang)
? iJoystickStickSwitch2[vkey - VK_STICK_SWITCH_1]
: iJoystickStickSwitch2[vkey - VK_STICK_SWITCH_1];
return ret;
}
....
}According to the written code, the correct version has to be the following:
ret = (lang)
? iJoystickStickSwitch2[vkey - VK_STICK_SWITCH_1]
: iJoystickStickSwitch1[vkey - VK_STICK_SWITCH_1];RT-Thread project, C
uint8_t can_receive_message_length(uint32_t can_periph,
uint8_t fifo_number)
{
uint8_t val = 0U;
if(CAN_FIFO0 == fifo_number){
val = (uint8_t)(CAN_RFIFO0(can_periph) & CAN_RFIFO_RFL0_MASK);
}else if(CAN_FIFO0 == fifo_number){
val = (uint8_t)(CAN_RFIFO1(can_periph) & CAN_RFIFO_RFL0_MASK);
}else{
/* illegal parameter */
}
return val;
}RT-Thread is a real-time open source OS for embedded devices. Here we see confusion between FIFO 0 and FIFO 1. And somewhere, someone's going to stumble upon a glitchy device.
The error is here:
if (CAN_FIFO0 == fifo_number){
....
}else if(CAN_FIFO0 == fifo_number){The second check always gives false. The correct version:
if (CAN_FIFO0 == fifo_number){
....
}else if(CAN_FIFO1 == fifo_number){Hive project, Java
private void
generateDateTimeArithmeticIntervalYearMonth(String[] tdesc) throws Exception {
String operatorName = tdesc[1];
String operatorSymbol = tdesc[2];
String operandType1 = tdesc[3];
String colOrScalar1 = tdesc[4];
String operandType2 = tdesc[5];
String colOrScalar2 = tdesc[6];
....
if (colOrScalar1.equals("Col") && colOrScalar1.equals("Column")) {
....
} else if (colOrScalar1.equals("Col") && colOrScalar1.equals("Scalar")) {
....
}The PVS-Studio analyzer indicates about 2 errors at once:
Variable names are definitely muddled up.
Shareaza project, C++
void CDownloadWithSources::MergeMetadata(const CXMLElement* pXML)
{
CQuickLock pLock( Transfers.m_pSection );
CXMLAttribute* pAttr1 =
m_pXML->GetAttribute(CXMLAttribute::schemaName);
CXMLAttribute* pAttr2 =
pXML->GetAttribute(CXMLAttribute::schemaName);
if (pAttr1 && pAttr2 &&
!pAttr1->GetValue().CompareNoCase(pAttr1->GetValue()))
....
}The correct version:
pAttr1->GetValue().CompareNoCase(pAttr2->GetValue())Note
Let's take a small break. There is concern, that when looking through a bunch of banal mistakes, we will forget why we do it.
The goal is not to scorn at someone else's code. All this is not the reason to play blame game and say: "Oh, my goodness, that's stupid!" This is the reason to pause to think!
Posts of our team are intended to show that none of us is immune to mistakes. The errors described in the article appear in code much more often than you might expect. It is also important that the probability of getting confused in 0, 1, 2 almost doesn't depend on the programmer's skill.
It is useful to realize that people tend to make mistakes. Without this, you can't take the next step in improving the quality and reliability of the code. Realizing that we all might be wrong, people begin to try to identify errors at the earliest stages, using coding standards, code reviews, unit tests, static and dynamic analyzers. That's very good.
Then why are we writing about obvious things? Unfortunately, based on numerous conversations with developers, we have to state that it is not always so clear to everyone. Many people have too high self-esteem and they simply don't allow the idea that they are able to make simple mistakes. It's sad.
If you are a teamlead/manager, I invite you to read this note.
Qt project, C++
AtomicComparator::ComparisonResult
IntegerComparator::compare(const Item &o1,
const AtomicComparator::Operator,
const Item &o2) const
{
const Numeric *const num1 = o1.as<Numeric>();
const Numeric *const num2 = o1.as<Numeric>();
if(num1->isSigned() || num2->isSigned())
....
}The correct version:
const Numeric *const num2 = o2.as<Numeric>();Android project, C++
static inline bool isAudioPlaybackRateEqual(
const AudioPlaybackRate &pr1,
const AudioPlaybackRate &pr2)
{
return fabs(pr1.mSpeed - pr2.mSpeed) <
AUDIO_TIMESTRETCH_SPEED_MIN_DELTA &&
fabs(pr1.mPitch - pr2.mPitch) <
AUDIO_TIMESTRETCH_PITCH_MIN_DELTA &&
pr2.mStretchMode == pr2.mStretchMode &&
pr2.mFallbackMode == pr2.mFallbackMode;
}There are two typos at once, due to which, variables pr2.mStretchMode and pr2.mFallbackMode are compared with themselves.
Boost project, C++
point3D operator/(const point3D &p1, const point3D &p2)
{
return point3D(p1.x/p2.x, p1.y/p2.y, p1.z/p1.z);
}At the very end, thanks to a typo, the p1.z variable is divided into itself.
Clang project, C++
bool haveSameType(QualType Ty1, QualType Ty2) {
return (Context.getCanonicalType(Ty1) ==
Context.getCanonicalType(Ty2) ||
(Ty2->isIntegerType() &&
Ty2->isIntegerType()));
}Yes, believe it or not, the PVS-Studio analyzer detects such bugs in compilers. The correct version:
(Ty1->isIntegerType() &&
Ty2->isIntegerType())Clang project, C++
Instruction *InstCombiner::visitXor(BinaryOperator &I) {
....
if (Op0I && Op1I && Op0I->isShift() &&
Op0I->getOpcode() == Op1I->getOpcode() &&
Op0I->getOperand(1) == Op1I->getOperand(1) &&
(Op1I->hasOneUse() || Op1I->hasOneUse())) {
....
}The correct version:
(Op0I->hasOneUse() || Op1I->hasOneUse())Qt project, C++
inline bool qCompare(QImage const &t1, QImage const &t2, ....)
{
....
if (t1.width() != t2.width() || t2.height() != t2.height()) {
....
}NCBI Genome Workbench project, C++
static bool s_PCRPrimerSetLess(const CPCRPrimerSet& s1, const CPCRPrimerSet& s2)
{
if (!s1.IsSet() && s1.IsSet()) {
return true;
} else if (s1.IsSet() && !s2.IsSet()) {
return false;
} else if (!s1.IsSet() && !s2.IsSet()) {
return false;
} else if (s1.Get().size() < s2.Get().size()) {
return true;
} else if (s1.Get().size() > s2.Get().size()) {
return false;
} else {
.....
}Error in the very first check. It should be like this:
if (!s1.IsSet() && s2.IsSet()) {NCBI Genome Workbench project, C++
CRef<CSeq_align> CNWAligner::Run(CScope &scope, const CSeq_loc &loc1,
const CSeq_loc &loc2, bool trim_end_gaps)
{
if ((!loc1.IsInt() && !loc1.IsWhole()) ||
(!loc1.IsInt() && !loc1.IsWhole()))
{
NCBI_THROW(CException, eUnknown,
"Only whole and interval locations supported");
}
....
}The first line of condition was copy-pasted, then the programmer got in a hurry and forgot to replace loc1 with loc2.
FlashDevelop project, C#
public void SetPrices(....)
{
UInt32 a0 = _choice.GetPrice0();
UInt32 a1 = _choice.GetPrice1();
UInt32 b0 = a1 + _choice2.GetPrice0(); // <=
UInt32 b1 = a1 + _choice2.GetPrice1();
....
}FreeCAD project, C++
inline void insEdgeVec(std::map<int,std::set<int> > &map,
int n1, int n2)
{
if(n1<n2)
map[n2].insert(n1);
else
map[n2].insert(n1);
};Regardless of the condition, one and the same action is executed. It would seem such a simple case. How was it possible to copy the line and not fix it? As you can see, it is possible.
LibreOffice project, C++
class SVX_DLLPUBLIC SdrMarkView : public SdrSnapView
{
....
const Point& GetRef1() const { return maRef1; }
const Point& GetRef2() const { return maRef1; }
....
};Classic Copy-Paste bug. The correct version:
const Point& GetRef2() const { return maRef2; }LibreOffice project, C++
bool CmpAttr(
const SfxPoolItem& rItem1, const SfxPoolItem& rItem2)
{
....
::boost::optional<sal_uInt16> oNumOffset1 =
static_cast<const SwFmtPageDesc&>(rItem1).GetNumOffset();
::boost::optional<sal_uInt16> oNumOffset2 =
static_cast<const SwFmtPageDesc&>(rItem1).GetNumOffset();
....
}And another classic Copy-Paste error :). In one fragment authors changed 1 for 2, but they forgot to do it in the other one.
LibreOffice project, C++
XMLTransformerOOoEventMap_Impl::XMLTransformerOOoEventMap_Impl(
XMLTransformerEventMapEntry *pInit,
XMLTransformerEventMapEntry *pInit2 )
{
if( pInit )
AddMap( pInit );
if( pInit )
AddMap( pInit2 );
}Here the mistake isn't about changing 1 for 2, here the author simply forgot to add 2 in the second condition.
Perhaps, you now feel a bit tired. Therefore, let's make some tea or coffee and we'll continue to explore the world of 0, 1, and 2 numbers.
Geant4 software project, C++
void G4VTwistSurface::GetBoundaryLimit(G4int areacode,
G4double limit[]) const
{
....
if (areacode & sC0Min1Max) {
limit[0] = fAxisMin[0];
limit[1] = fAxisMin[1];
} else if (areacode & sC0Max1Min) {
limit[0] = fAxisMax[0];
limit[1] = fAxisMin[1];
} else if (areacode & sC0Max1Max) {
limit[0] = fAxisMax[0];
limit[1] = fAxisMax[1];
} else if (areacode & sC0Min1Max) {
limit[0] = fAxisMin[0];
limit[1] = fAxisMax[1];
}
....
}I hope you took the advice and had some rest. Are you ready to find the error in this code now?
Congrats to those who managed to do it! You did great!
However, I understand those who got a bit lazy. Reviewing such code is very tedious and you probably want to somehow quickly move on to checking something more interesting. Static analyzers are excellent for such cases, because they don't get tired.
The error is that these two checks are the same:
if (areacode & sC0Min1Max) {
} else if (areacode & sC0Min1Max) {If you carefully review the code, it becomes clear that the very first check is erroneous. The correct version:
if (areacode & sC0Min1Min) {
} else if (areacode & sC0Max1Min) {
} else if (areacode & sC0Max1Max) {
} else if (areacode & sC0Min1Max) {CryEngine V project, C++
bool
CompareRotation(const Quat& q1, const Quat& q2, float epsilon)
{
return (fabs_tpl(q1.v.x - q2.v.x) <= epsilon)
&& (fabs_tpl(q1.v.y - q2.v.y) <= epsilon)
&& (fabs_tpl(q2.v.z - q2.v.z) <= epsilon) // <=
&& (fabs_tpl(q1.w - q2.w) <= epsilon);
}TortoiseGit project, C++
void CGitStatusListCtrl::OnContextMenuList(....)
{
....
if( (!this->m_Rev1.IsEmpty()) ||
(!this->m_Rev1.IsEmpty()) )
....
}Geant4 software project, C++
G4double G4MesonAbsorption::
GetTimeToAbsorption(const G4KineticTrack& trk1,
const G4KineticTrack& trk2)
{
....
if(( trk1.GetDefinition() == G4Neutron::Neutron() ||
trk1.GetDefinition() == G4Neutron::Neutron() ) &&
sqrtS>1.91*GeV && pi*distance>maxChargedCrossSection)
return time;
....
}MonoDevelop project, C#
private bool MembersMatch(ISymbol member1, ISymbol member2)
{
....
if (member1.DeclaredAccessibility !=
member1.DeclaredAccessibility
|| member1.IsStatic != member1.IsStatic)
{
return false;
}
....
}As you can see, the above code fragments go unexplained so far. Actually, there is nothing to explain. You can only groan and offer your empathy.
Dolphin Emulator project, C++
bool IRBuilder::maskedValueIsZero(InstLoc Op1, InstLoc Op2) const
{
return (~ComputeKnownZeroBits(Op1) &
~ComputeKnownZeroBits(Op1)) == 0;
}RunAsAdmin Explorer Shim project, C++
bool IsLuidsEqual(LUID luid1, LUID luid2)
{
return (luid1.LowPart == luid2.LowPart) &&
(luid2.HighPart == luid2.HighPart);
}IT++, C++
Gold::Gold(const ivec &mseq1_connections,
const ivec &mseq2_connections)
{
....
it_assert(mseq1.get_length() == mseq1.get_length(),
"Gold::Gold(): dimension mismatch");
}QuantLib, C++
Distribution ManipulateDistribution::convolve(
const Distribution& d1, const Distribution& d2) {
....
QL_REQUIRE (d1.xmin_ == 0.0 && d1.xmin_ == 0.0,
"distributions offset larger than 0");
....
}Samba project, C++
static bool samu_correct(struct samu *s1, struct samu *s2)
{
....
} else if (s1_len != s1_len) {
DEBUG(0, ("Password history not written correctly, "
"lengths differ, want %d, got %d\n",
s1_len, s2_len));
....
}Mozilla Firefox project, C++
static PRBool IsZPositionLEQ(nsDisplayItem* aItem1,
nsDisplayItem* aItem2,
void* aClosure) {
if (!aItem1->GetUnderlyingFrame()->Preserves3D() ||
!aItem1->GetUnderlyingFrame()->Preserves3D()) {
return IsContentLEQ(aItem1, aItem2, aClosure);
}
....
}Haiku Operation System, C++
void trans_double_path::reset()
{
m_src_vertices1.remove_all();
m_src_vertices2.remove_all();
m_kindex1 = 0.0; // <=
m_kindex1 = 0.0; // <=
m_status1 = initial;
m_status2 = initial;
}Qt project, C++
Ok, now let's get to more complicated cases. Try to find the error here just for the sake of interest:
static ShiftResult shift(....)
{
....
qreal l = (orig->x1 - orig->x2)*(orig->x1 - orig->x2) +
(orig->y1 - orig->y2)*(orig->y1 - orig->y1) *
(orig->x3 - orig->x4)*(orig->x3 - orig->x4) +
(orig->y3 - orig->y4)*(orig->y3 - orig->y4);
....
}Here's the picture not to see the answer straight away so that you got a chance to think about the fragment.
Right, orig->y1 - orig->y2 has to be written instead of orig->y1 - orig->y1.
.NET Compiler Platform project, C#
public void IndexerMemberRace()
{
....
for (int i = 0; i < 20; i++)
{
....
if (i % 2 == 0)
{
thread1.Start();
thread2.Start();
}
else
{
thread1.Start();
thread2.Start();
}
....
}
....
}That's an interesting case. For testing purposes, you want to run threads in a different order. However, due to a typo, threads always start in the same way, so the test checks less than it should.
The correct version:
if (i % 2 == 0)
{
thread1.Start();
thread2.Start();
}
else
{
thread2.Start();
thread1.Start();
}Samba project, C
static int compare_procids(const void *p1, const void *p2)
{
const struct server_id *i1 = (struct server_id *)p1;
const struct server_id *i2 = (struct server_id *)p2;
if (i1->pid < i2->pid) return -1;
if (i2->pid > i2->pid) return 1;
return 0;
}The comparison function will never return 1, as the i2->pid > i2->pid condition is pointless.
Naturally, that's a trivial typo, in fact, the following has to be written:
if (i1->pid > i2->pid) return 1;ChakraCore project, C++
The last case in this section. Yippee!
bool Lowerer::GenerateFastBrSrEq(....,
IR::RegOpnd * srcReg1,
IR::RegOpnd * srcReg2,
....)
{
....
else if (srcReg1 && (srcReg1->m_sym->m_isStrConst))
....
else if (srcReg1 && (srcReg1->m_sym->m_isStrConst))
....
}Now I'm going to mention error patterns related to 0, 1, 2 usage with fewer examples.
ROOT project, C++
Int_t TProofMonSenderML::SendSummary(TList *recs, const char *id)
{
....
if (fSummaryVrs == 0) {
if ((dsn = recs->FindObject("dataset"))) recs->Remove(dsn);
} else if (fSummaryVrs == 0) {
....
}It's strange to compare the fSummaryVrs variable with 0 twice.
.NET CoreCLR, C#
void PutIA64Imm22(UINT64 * pBundle, UINT32 slot, INT32 imm22)
{
if (slot == 0) // <=
{
....
}
else if (slot == 1)
{
....
}
else if (slot == 0) // <=
{
....
}
....
}FFmpeg project, C
static int imc_decode_block(....)
{
....
if (stream_format_code & 0x1)
imc_decode_level_coefficients_raw(....);
else if (stream_format_code & 0x1)
imc_read_level_coeffs_raw(....);
....
}Previously, we have looked at cases where an index or a name is incorrect. And here is a situation where you can't immediately tell how to classify the error. This example could be attributed to both chapters. So I decided to bring it separately.
Mesa 3D Graphics Library project, C++
bool
ir_algebraic_visitor::reassociate_constant(....)
{
....
if (ir1->operands[0]->type->is_matrix() ||
ir1->operands[0]->type->is_matrix() ||
ir2->operands[1]->type->is_matrix() ||
ir2->operands[1]->type->is_matrix())
return false;
....
}This code can be fixed as follows:
if (ir1->operands[0]->type->is_matrix() ||
ir1->operands[1]->type->is_matrix() ||
ir2->operands[0]->type->is_matrix() ||
ir2->operands[1]->type->is_matrix())As well as in this way:
if (ir1->operands[0]->type->is_matrix() ||
ir2->operands[0]->type->is_matrix() ||
ir1->operands[1]->type->is_matrix() ||
ir2->operands[1]->type->is_matrix())Sometimes 0 is superfluous and harmful. Because of it, the number can turn into an octal one, in the place where it shouldn't. Or spoil the format string.
These errors aren't suitable for this article, but I think they are worth mentioning. I won't give you the code with these errors in the article, but if you're interested, you can check them out here:
Haiku Operation System, C++
int
UserlandFS::KernelEmu::new_path(const char *path, char **copy)
{
....
// append a dot, if desired
if (appendDot) {
copiedPath[len] = '.';
copiedPath[len] = '\0';
}
....
}Here is the correct version:
copiedPath[len] = '.';
copiedPath[len + 1] = '\0';Note. The case when one forgets to add 1 is not uncommon. I remember exactly that I have seen such cases quite often. However, when I wanted to collect such examples for an article, I found only this example. I'm sorry I can't scare you with more errors. I do apologize.
Most often functions for building strings operate with a small number of arguments. So it turns out that errors relate to the usage of {0}, {1} or {2}.
Azure PowerShell project, C#
protected override void ProcessRecordInternal()
{
....
if (this.ShouldProcess(this.Name,
string.Format("Creating Log Alert Rule '{0}' in resource group {0}",
this.Name, this.ResourceGroupName)))
{
....
}
....
}The author made a typo and wrote {0} twice. As a result, the this.Name name will be inserted in the string twice. As for the this.ResourceGroupName name, it won't get into the created string.
Mono project, C#
void ReadEntropy ()
{
if (reader.IsEmptyElement)
throw new XmlException (
String.Format ("WS-Trust Entropy element is empty.{2}",
LineInfo ()));
....
}The above code is so weird. You are supposed to insert something that doesn't exist. Most likely, this code underwent failed refactoring and the logic was violated.
Xenko project, C#
public string ToString(string format,
IFormatProvider formatProvider)
{
if (format == null)
return ToString(formatProvider);
return string.Format(
formatProvider,
"Red:{1} Green:{2} Blue:{3}",
R.ToString(format, formatProvider),
G.ToString(format, formatProvider),
B.ToString(format, formatProvider));
}The programmer forgot that the numbering begins with {0}, not with {1}. Correct code:
return string.Format(
formatProvider,
"Red:{0} Green:{1} Blue:{2}",
R.ToString(format, formatProvider),
G.ToString(format, formatProvider),
B.ToString(format, formatProvider));.NET Compiler Platform project, C#
private void DumpAttributes(Symbol s)
{
....
Console.WriteLine("{0} {1} {2}", pa.ToString());
....
}Arguments are clearly not enough.
I had to demonstrate quite many examples to show that typos related to 0, 1, and 2 deserve special attention.
If I'd just said: "It's easy to confuse o1 and o2", you would have agreed, but wouldn't have given it special attention, as you're giving it now after reading or at least looking through the article.
Now you're forewarned, and that's good. Forewarned is forearmed. From now on you'll be more attentive during code reviews and will pay extra attention to variables with 0, 1, 2 in the names.
It's difficult to give certain recommendations on code formatting so as to avoid the above errors. As you have seen, errors occur even in such simple code, where there is actually nothing to format.
Therefore, I won't call to avoid 0, 1, 2 and give variables long names. If you start writing First/Second/Left/Right instead of numbers, then the temptation to copy the name or expression will be even greater. Perhaps this recommendation won't ultimately reduce, but will increase the number of errors.
However, when you write a lot of similar code, the recommendation of "table code formatting" is still relevant. Table formatting doesn't guarantee the absence of typos, but helps to notice them more easily and faster. See the chapter 13 from the mini-book "The ultimate question of programming, refactoring, and everything" for more details.
There's another piece of good news. All of the errors discussed in this article are detected by the PVS-Studio static code analyzer. Accordingly, by introducing static analysis tools into the development process, you will be able to identify many typos at the earliest stage.
Thank you for your attention. I hope you were interested and scared. I wish you reliable code and less errors with 0, 1, 2, so that Freddy didn't come to you.
0
0
0
0