Get best of the week

Zero, one, two, Freddy will pick you up

Picture 1

Here is a continuation of a series of articles that can be entitled "horrors for programmers." This time we will talk about a typical typo pattern associated with the use of the numbers 0, 1, 2. It doesn’t matter if you write in C, C ++, C # or Java. If you use constants 0, 1, 2, or if these numbers are contained in variable names, then, most likely, Freddy will visit you at night. Read and do not say later that you were not warned.


Introduction


I continue a series of articles devoted to noticed patterns in how people make mistakes. Previous publications:
  1. Last line effect
  2. The most dangerous function in the world of C / C ++
  3. Evil lives in comparison functions

This time the pattern was noticed not by me, but by my colleague Svyatoslav Razmyslov. He noted that he constantly described in his articles problems in which variables containing numbers 1 and 2 appear in his name. Svyatoslav suggested that I study this topic in more detail and it really turned out to be very fruitful. It turned out that our collection of errors contains a large number of code fragments that are incorrect due to the fact that people get confused in the indices 0, 1, 2 or in the names of variables containing such numbers. A new interesting regularity is revealed, which will be discussed below. I am grateful to Svyatoslav for the prompt to investigate this topic and dedicate this article to him.

Figure 14

Svyatoslav Razmyslov, manager, attentive bug catcher and just a talented person.

What is the purpose of this article? Show how easy we all make mistakes and typos. If programmers are warned, they will be more attentive in the process of code reviews, focusing on the ill-fated 0, 1, 2. Also, programmers will be better able to feel the value of static code analyzers that help to notice such errors. It's not about PVS-Studio advertising (although it is also :). Until now, many programmers find static analysis superfluous, preferring to concentrate on their own accuracy and code reviews. Unfortunately, trying to write code without errors is good, but not enough. This article will once again demonstrate this clearly.

No one is immune from such errors. Below you will see epic bloopers in even such well-known projects as Qt, Clang, Hive, LibreOffice, Linux Kernel, .NET Compiler Platform, XNU kernel, Mozilla Firefox. And these are not some exotic rare errors, but the most frequent ones. Not convinced? Then let's get started!

“Chatter is worthless! Show me the bugs! ”

© modified quote by Linus Torvalds.

Typos in constants when indexing arrays


As a rule, in our articles we give warnings with the help of which errors are found. This time I will omit these warnings, as without them the error will be easily noticeable and understandable. However, although these errors are immediately evident in a short piece of code, they know how to hide in the code of projects.

Let's start with the mistakes when there is confusion with the numeric literals used to index arrays. Despite the banality of these errors, there are many of them and they are not revealed in the laboratory work of students.

XNU kernel project, C language
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 forgot to correct the index. As I understand it, it should be written here:
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 line was copied, but forgot to fix 0 by 1. Only the string literal was corrected.

One may ask a philosophical question, how can one make such a mistake in a four-line function? All is possible. Here it is, programming.

Quake-III-Arena Project, C language
int VL_FindAdjacentSurface(....)
{
  ....
  if (fabs(dir[0]) > test->radius ||
      fabs(dir[1]) > test->radius ||
      fabs(dir[1]) > test->radius)
  {
  ....
}

In the copied line, they forgot to replace dir [1] with dir [2] . As a result, the value along the Z axis is not controlled.

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, you can manage to go beyond the boundary of an array when it is initialized.

Figure 8


Godot Engine Project, C ++
Array PhysicsDirectSpaceState::_cast_motion(....)
{
  ....
  Array ret(true);
  ret.resize(2);
  ret[0]=closest_safe;
  ret[0]=closest_unsafe;
  return ret;
}

No comment required.

Asterisk project, C language
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 the same type of blocks, the error, as a rule, is located in the underlying one. Prior to this, all the cases considered were just that. Here the typo is in an unusual place, namely, in the first block. Why it happened is hard to say. I have no choice but to bring a picture of a unicorn shrugging:

Figure 9


Open CASCADE Technology Project, C ++
inline void Prepend(const Standard_Integer theIndex)
{
  if (myIndex[1] >= 0)
    Standard_OutOfRange::Raise ("BRepMesh_PairOfIndex....");

  myIndex[1] = myIndex[0];
  myIndex[1] = theIndex;
}

Two times in the same cell of the array, different values ​​are copied. An obvious error, but how to fix it was not clear to me, since the project code is not familiar to me. So I just looked at how the developers corrected the code after our team pointed out this error to them. The correct option:
myIndex[1] = myIndex[0];
myIndex[0] = theIndex;

Trans-Proteomic Pipeline Project, 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 worry that research packages contain such errors. Trans-Proteomic Pipeline is designed to solve problems in the field of biology. This can be decided and "investigated." This package generally find a lot of interesting things: check in 2012 , check 2013 . Perhaps you can again try to look at this project.

ITK project, C ++ language

We are faced with another project for conducting research in the field of medicine: Medicine Insight Segmentation and Registration Toolkit (ITK). The project is different, but the mistakes 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;
  ....
}

Clean Copy-Paste.

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);
  ....
}

Most likely, the crBackgnd constant should be written to the cols cell [2] .

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 element of the size [1] array is double checked , and the element of size [2] is not checked. This is how strange artifacts appear on 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;
  ....
}

It is directly felt that the expression cmptlut [0] <0 was duplicated twice by copying, but corrected zero in only one place.

Visualization Toolkit (VTK) Project, C ++
void vtkImageStencilRaster::PrepareForNewData(....)
{
  ....
  if (allocateExtent &&
      allocateExtent[1] >= allocateExtent[1])
  ....
}

Hereinafter, I will not comment on many such errors. What is there to comment on? The main thing when looking at such code fragments is to feel that although the error is simple, this does not mean that it will be noticed by the programmer.

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 to write code faster. It is difficult to explain in another way how he made a mistake three times. Elements of the array are subtracted from themselves. The result is that this code is equivalent:
inZPtr +=
  (0)*inIncs[0] * data_type_size +
  (0)*inIncs[1] * data_type_size +
  (0)*inIncs[2] * data_type_size;

However, this code can be reduced even further:
inZPtr += 0;

Sumptuously. There is a long, serious looking expression in the code that, in fact, does nothing. I love such cases.

Visualization Toolkit (VTK) project, C ++ language

A similar case of hasty writing code.
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 comparison newPos [2]! = OldPos [2] is repeated twice .

ADAPTIVE Communication Environment (ACE) Project, 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 must verify that two slashes after the colon are encountered. In other words, the substring ": //" is searched. Due to a typo, the check is "blind" and is ready to count any character as a second slash.

IPP Samples Project, 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]));
  ....
}

A typo lies here, in the arguments passed to the macro:
IPP_MIN(m_cur.AcRate[2],m_cur.AcRate[2])

It turns out that a minimum of two identical values ​​is selected. In fact, it should be written:
IPP_MIN(m_cur.AcRate[2],m_cur.AcRate[3])

By the way, this code can demonstrate the usefulness of the standard library. If you write like this:
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));

That code will become shorter and less prone to errors. Actually, the less the same type of code, the more likely it will be written correctly.

Project 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]);
  ....
}

The correct expression is:
int ztrend = sgn(buffer[samplesleft - WindowSizeInt-2]-
                 buffer[samplesleft - WindowSizeInt-1]);

PDFium project, C ++ language
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;
}

A series of actions to initialize the structure are duplicated. Those lines marked with comment // 2 can be deleted, and nothing will change. I doubted whether to include this piece of code in the article. This is not quite a mistake, and not quite with the indices. However, this extra code most likely appeared precisely because the programmer got confused in all these class members and indices 1, 2. So, I think this piece of code is suitable for demonstrating how easy it is to get confused in numbers.

CMake project, C

The code discussed below was not written by the CMake developers, but was borrowed. Judging by the comment at the beginning of the file, the utf8_encode functionIt was written by Tim Kientzle back in 2007. Since then, this function has been wandering from project to project, and there are a lot of them. I did not study the issue of the original source, since this is not the point now. Since this code is in the CMake project, then the error applies to CMake.
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 kind of confusion with the indexes. Twice there is a record in the array element p [1] . If you study the code in the neighborhood, it becomes clear that the correct code should be like 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 discussed in this chapter are related to C or C ++ code. No C # or Java Code!

This is very interesting, I did not expect this. In my opinion, the typos considered do not depend on the language. And in the following chapters, errors in code in other languages ​​will indeed appear. I think this is just a coincidence. The PVS-Studio analyzer started much later to support C # / Java languages ​​compared to C / C ++, and we simply did not manage to accumulate the corresponding error examples in the database.

However, the observation is still interesting. Apparently, C and C ++ programmers like to use numbers 0, 1 and 2 more when working with arrays :).

Misspellings in names


This will be the largest section. It's very easy for people to get confused by names like a1 and a2 . It seems like you can get confused here? Can. Easy. And now the reader will be able to verify this.

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 compare function compare takes two objects: o1 and o2 . But due to a typo, only o2 is used further .

Interestingly, thanks to Copy-Paste, this error migrated to another function:
@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;
}

Figure 10


Infer.NET project, C # language
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 someone did not immediately notice a typo, then you need to look at the line where the l2 variable is initialized . Should use exp2 .

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;
  }
  ....
}

Correctly:
nFPSThreshold1 = 60;
nFPSThreshold2 = 50;

Linux Kernel project, C language

By the way, typos can be not only in variable names, but also in macro names. Now there will be several such 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, a mask called BIT1 is used twice , which makes the second check pointless. The body of the second commented conditional statement will never be executed.

CMaNGOS Project, C ++
void AttackedBy(Unit* pAttacker) override
{
  ....
  DoScriptText(urand(0, 1) ?
               SAY_BELNISTRASZ_AGGRO_1 :
               SAY_BELNISTRASZ_AGGRO_1,
               m_creature, pAttacker);
  ....
}

Random behavior was planned in the game, but the same constant SAY_BELNISTRASZ_AGGRO_1 is always selected .

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;
  }
  ....
}

Judging by the code written next to it, the correct option should be like this:
ret = (lang)
  ? iJoystickStickSwitch2[vkey - VK_STICK_SWITCH_1]
  : iJoystickStickSwitch1[vkey - VK_STICK_SWITCH_1];

RT-Thread Project, C language
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 an open-source, real-time operating system for embedded devices. Here we see the confusion between FIFO 0 and FIFO 1. And somewhere, someone will encounter a buggy device.

Figure 11


The error is here:
if      (CAN_FIFO0 == fifo_number){
....
}else if(CAN_FIFO0 == fifo_number){

The second check always gives false. Correctly:
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 immediately indicates 2 errors:
  1. A string stored in colOrScalar1 cannot equal both Col and Column strings;
  2. The string stored in colOrScalar1 cannot equal the Col and Scalar strings at the same time.

There is clearly confusion about variable names.

Shareaza project, C ++ language
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()))
    ....
}

Correctly:
pAttr1->GetValue().CompareNoCase(pAttr2->GetValue())

Note

Let's take a short pause. There is a fear that looking through a mountain of banal mistakes, we will forget why we are doing this.

The task is not to laugh at someone else's code. All this is not a reason to poke your finger and say: "Ha ha, well, you have to." This reason to think!

The publications of our team are designed to show that none of us is immune from mistakes. Errors described in the article appear in the code much more often than you might expect. It is also important that the probability of getting lost in 0, 1, 2 is almost independent of the qualifications of the programmer.

It is useful to realize that people tend to make mistakes. Without this, you cannot take the next step in improving the quality and reliability of the code. Understanding that we are all mistaken, people begin to try to identify errors at the earliest stages, using coding standards, code reviews, unit tests, static and dynamic analyzers. It is very good.

Why are things so understandable written? Unfortunately, communicating with a large number of developers, we are forced to state that it is not always so clear to everyone. Many have too high self-esteem and they simply do not allow the thought that they are capable of making simple mistakes. It is sad.

If you are a team leader / manager, then I invite you to familiarize yourself with this note at the same time .

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())
  ....
}

Correctly:
const Numeric *const num2 = o2.as<Numeric>();

Android project, C ++ language
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;
}

Two typos at once, due to which the 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, they sealed up and divided the variable p1.z itself.

Clang Project, C ++
bool haveSameType(QualType Ty1, QualType Ty2) {
  return (Context.getCanonicalType(Ty1) ==
          Context.getCanonicalType(Ty2) ||
          (Ty2->isIntegerType() &&
           Ty2->isIntegerType()));
}

Yes, yes, PVS-Studio analyzer finds similar errors in compilers. Correctly:
(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())) {
  ....
}

Correctly:
(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 written:
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 the condition was propagated, but then the programmer hastened 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, the same action is performed. It would seem such a simple case. How could you copy a line and not fix it? Can.

LibreOffice Project, C ++
class SVX_DLLPUBLIC SdrMarkView : public SdrSnapView
{
  ....
  const Point& GetRef1() const { return maRef1; }
  const Point& GetRef2() const { return maRef1; }
  ....
};

Classical Copy-Paste error. Correctly:
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 one more classic Copy-Paste error :). In one place 1 to 2 corrected, and in another they forgot.

LibreOffice Project, C ++
XMLTransformerOOoEventMap_Impl::XMLTransformerOOoEventMap_Impl(
        XMLTransformerEventMapEntry *pInit,
        XMLTransformerEventMapEntry *pInit2 )
{
  if( pInit )
    AddMap( pInit );
  if( pInit )
    AddMap( pInit2 );
}

There is no mistake in replacing 1 with 2, but just did not add 2 to the second condition.

Figure 12


You may be a little tired. Therefore, I propose to make tea or coffee, and we will continue our acquaintance with the world of numbers 0, 1 and 2.

Geant4 software project, C ++ language
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 rested. Ready to find an error in this code?

Congratulations to readers who have noticed a mistake. You are great!

Those who are too lazy to search, I also understand those. The review of such a code is very tedious and there is a desire to somehow quickly go check something more interesting. This is where static analyzers help a lot 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 study the code, it becomes clear that the very first check is erroneous. Correctly:
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 ++ language
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, while the code fragments go without explanation. Actually, there is nothing to explain here. You can only sigh.

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 ++ project, C ++ language
Gold::Gold(const ivec &mseq1_connections,
           const ivec &mseq2_connections)
{
  ....
  it_assert(mseq1.get_length() == mseq1.get_length(),
            "Gold::Gold(): dimension mismatch");
}

QuantLib Project, 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 Project, 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;
}

The Qt project, C ++

Ok, now let's get a little more complicated. For fun, try to find the error here:
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);
  ....
}

A picture, so as not to see the answer immediately, and had the opportunity to think.

Figure 13


That's right, instead of orig-> y1 - orig-> y1 should be written orig-> y1 - orig-> y2 .

.NET Compiler Platform Project, C # Language
public void IndexerMemberRace()
{
  ....
  for (int i = 0; i < 20; i++)
  {
    ....
    if (i % 2 == 0)
    {
      thread1.Start();
      thread2.Start();
    }
    else
    {
      thread1.Start();
      thread2.Start();
    }
    ....
  }
  ....
}

Interesting case. For testing purposes, you need to run threads in a different order. However, due to a typo, the threads always start the same way, as a result of which the test checks less than it should.

Correctly:
if (i % 2 == 0)
{
  thread1.Start();
  thread2.Start();
}
else
{
  thread2.Start();
  thread1.Start();
}

Samba Project, C language
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, since the condition i2-> pid> i2-> pid does not make sense.

Naturally, this is an ordinary typo, and in fact it should be written:
if (i1->pid > i2->pid) return 1;

ChakraCore Project, C ++

The last case in this chapter. Hooray!
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))
  ....
}


Other errors


Now let's talk about less numerous error patterns associated with the use of numbers 0, 1, 2.

Typos in conditions where the constant 0/1/2 is explicitly used


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 weird comparing fSummaryVrs to 0. twice .

.NET CoreCLR project, C #
void PutIA64Imm22(UINT64 * pBundle, UINT32 slot, INT32 imm22)
{
  if (slot == 0)             // <=
  {
    ....
  }
  else if (slot == 1)
  {
    ....
  }
  else if (slot == 0)        // <=
  {
    .... 
  }
  ....
}

FFmpeg Project, C language
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(....);
  ....
}


Zip / Name


Earlier we considered cases when the index or name is incorrect. And here is a situation where you won’t immediately tell how to classify the error. This example could be attributed to both one and the other chapter. Therefore, 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 like this:
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())

And you can fix it like this:
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())


Extra 0


Sometimes 0 is redundant and harmful. Because of it, the number can turn into octal where it is not needed. Or spoil the format string.

The errors mentioned are not suitable for this article, but I think that it is worth mentioning them. I will not give a code with these errors in the article, but if interested, you can look at them here:
  • V536 Be advised that the utilized constant value is represented by an octal form, examples ;
  • V638 A terminal null is present inside a string. The '\ 0xNN' characters were encountered. Probably meant: '\ xNN', examples .


Forgot to write +1


Haiku Operation System Project, C ++
int
UserlandFS::KernelEmu::new_path(const char *path, char **copy)
{
  ....
  // append a dot, if desired
  if (appendDot) {
    copiedPath[len] = '.';
    copiedPath[len] = '\0';
  }
  ....
}

The correct option:
copiedPath[len] = '.';
copiedPath[len + 1] = '\0';

Note. The situation when they forgot to add a unit is not at all rare. I remember exactly that more than once I met such cases. However, when I wanted to type similar examples for the article, I found only this example. I'm sorry that I can not scare you with these errors more. I apologize.

Formatting Errors (C #)


Most often, functions for constructing strings operate with a small number of arguments. So it turns out that most often errors are associated with the use 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)))
  {
    ....
  }
  ....
}

Sealed up and wrote {0} twice. As a result, the name this.Name will be inserted into the string twice . But the name this.ResourceGroupName will not 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 ()));
  ....
}

This is generally strange. You need to insert what is not. Most likely, this code underwent unsuccessful refactoring and turned out to be broken.

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 numbering starts with {0}, not {1}. The correct code is:
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.

Conclusions and recommendations


I had to demonstrate so many examples to show that typos related to 0, 1, and 2 deserve special attention.

If I simply said: “It is easy to confuse o1 and o2”, you would agree, but not attach to it the meaning that you attach now, after reading or at least scrolling through the article.

Now you are warned, and that’s good. Forewarned is forearmed. Now you will be more attentive to code reviews and pay extra attention to variables, in the names of which you will see 0, 1, 2. It is

difficult to give some recommendations on how the code is designed to avoid such errors. As you have seen, errors are found even in such simple code, where there is nothing to issue.

Therefore, I will not urge you to avoid 0, 1, 2 and give variables long names. If instead of numbers you start to write First / Second / Left / Right and so on, the temptation to copy the name or expression will be even greater. Perhaps such a recommendation will ultimately not reduce, but increase the number of errors.

Nevertheless, when you write a lot of the same type of code, the recommendation of "tabular code design" is still relevant. Tabular formatting does not guarantee the absence of typos, but makes them easier and faster to notice. See chapter N13 in the mini-book " The Main Question of Programming, Refactoring, and All That ."

There is one more good news. All errors discussed in this article were found using the PVS-Studio static code analyzer.. Accordingly, introducing static analysis tools into the development process, you can identify many typos at an early stage.

Thank you for the attention. I hope you were interested and scared. I wish you a reliable code and less error with 0, 1, 2, so that Freddy does not come to you.



If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. Zero, one, two, Freddy's coming for you .

More articles:


All Articles