Прежде чем перейти к статье, хочу вам представить, экономическую онлайн игру Brave Knights, в которой вы можете играть и зарабатывать. Регистируйтесь, играйте и зарабатывайте!
Перед вами продолжение серии статей, которую можно озаглавить «ужасы для программистов». В этот раз речь пойдёт о типовом паттерне опечаток, связанном с использованием чисел 0, 1, 2. Неважно, пишете вы на C, C++, C# или Java. Если вы используете константы 0, 1, 2, или если эти числа содержатся в именах переменных, то, скорее всего, Фредди заглянет к вам ночью в гости. Читайте и не говорите потом, что вас не предупреждали.
Введение
Я продолжаю серию статей, посвященных замеченным закономерностям в том, как люди допускают ошибки. Предыдущие публикации:
- Эффект последней строки
- Самая опасная функция в мире C/C++
- Зло живёт в функциях сравнения
В этот раз закономерность заметил не я, а мой коллега Святослав Размыслов. Он обратил внимание, что постоянно в своих статьях описывает проблемы, в которых фигурируют переменные, содержащие в своём имени числа 1 и 2. Святослав предложил мне подробнее исследовать эту тему и она действительно оказалась весьма плодотворной. Выяснилось, что в нашей коллекции ошибок содержится большое число фрагментов кода, неправильных из-за того, что люди путаются в индексах 0, 1, 2 или в именах переменных, содержащих такие числа. Выявлена новая интересная закономерность, о которой и пойдёт речь ниже. Я благодарен Святославу за подсказку исследовать эту тему и посвящаю эту статью ему.
Святослав Размыслов, менеджер, внимательный ловец багов и просто талантливый человек.
Какова цель этой статьи? Показать, как легко все мы ошибаемся и допускаем опечатки. Если программисты будут предупреждены — они будут внимательнее в процессе обзоров кода, сосредотачивая своё внимание на злополучных 0, 1, 2. Также программисты лучше смогут ощутить ценность статических анализаторов кода, которые помогают заметить такие ошибки. Дело не в рекламе PVS-Studio (хотя в ней тоже :). До сих пор многие программисты считают статический анализ лишним, предпочитая концентрироваться на собственной аккуратности и обзорах кода. К сожалению, стараться писать код без ошибок — это хорошо, но недостаточно. Эта статья в очередной раз это ярко продемонстрирует.
Никто не застрахован от таких ошибок. Ниже вы увидите эпичные ляпы в даже таких известных проектах как Qt, Clang, Hive, LibreOffice, Linux Kernel, .NET Compiler Platform, XNU kernel, Mozilla Firefox. Причем это не какие-то экзотические редкие ошибки, а самые что ни на есть частые. Не убедил? Тогда приступим!
«Болтовня ничего не стоит! Покажите мне баги!»
© переделанная цитата Linus Torvalds.
Опечатки в константах при индексации массивов
Как правило, в наших статьях мы приводим предупреждения, с помощью которых найдены ошибки. В этот раз я опущу эти предупреждения, так как и без них ошибка будет легко заметна и понятна. Впрочем, хотя эти ошибки сразу бросаются в глаза в коротком фрагменте кода, они отлично умеют прятаться в коде проектов.
Начнём с ошибок, когда возникает путаница с числовыми литералами, используемыми для индексации массивов. Несмотря на банальность этих ошибок, их много и обнаруживаются они отнюдь не в лабораторных работах студентов.
Проект XNU kernel, язык 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;
}
....
}Строчку скопировали, но поправить индекс забыли. Как я понимаю, здесь должно быть написано:
wrap.Seal_Alg[0] = 0xff;
wrap.Seal_Alg[1] = 0xff;Проект LibreOffice, язык 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;
}Как и в предыдущем случае, строчку скопировали, но забыли исправить 0 на 1. Исправили только строковый литерал.
Можно задаться философским вопросом, как можно допустить такую ошибку в функции из четырех строк? Можно и всё. Вот такое оно, программирование.
Проект Quake-III-Arena, язык C
int VL_FindAdjacentSurface(....)
{
....
if (fabs(dir[0]) > test->radius ||
fabs(dir[1]) > test->radius ||
fabs(dir[1]) > test->radius)
{
....
}В скопированной строчке забыли заменить dir[1] на dir[2]. Как результат — значение по оси Z не контролируется.
Проект OpenCOLLADA, язык C++
struct short2
{
short values[2];
short2(short s1, short s2)
{
values[0] = s1;
values[2] = s2;
}
....
};Да, даже в таком коротком конструкторе можно умудриться выйти за границу массива при его инициализации.
Проект Godot Engine, язык C++
Array PhysicsDirectSpaceState::_cast_motion(....)
{
....
Array ret(true);
ret.resize(2);
ret[0]=closest_safe;
ret[0]=closest_unsafe;
return ret;
}Комментария не требуется.
Проект 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;
....
}При написании однотипных блоков, ошибка, как правило, располагается в нижележащем. До этого все рассмотренные случаи были как раз такими. Здесь же опечатка находится в необычном месте, а именно, в первом блоке. Почему так получилось, сказать сложно. Мне не остаётся ничего иного, как просто привести картинку с единорогом, пожимающим плечами:
Проект 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;
}Два раза в одну и туже ячейку массива копируются разные значения. Явная ошибка, но как её исправить мне было непонятно, так как код проекта мне не знаком. Поэтому я просто посмотрел, как разработчики поправили код после того, как наша команда указала им на эту ошибку. Правильный вариант:
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;
}
....
}Я переживаю, что пакеты для научных исследований содержат такие ошибки. Trans-Proteomic Pipeline предназначен для решения задач в сфере биологии. Можно такого нарешать и «наисследовать». В этом пакете вообще находилось много интересного: проверка 2012 года, проверка 2013 года. Пожалуй, можно вновь попробовать взглянуть на этот проект.
Проект ITK, язык C++
Перед нами другой проект для проведения исследований в области медицины: Medicine Insight Segmentation and Registration Toolkit (ITK). Проект другой, а ошибки те же самые.
template< typename TCoordRepType >
void
VoronoiDiagram2D< TCoordRepType >::SetOrigin(PointType vorsize)
{
m_VoronoiBoundaryOrigin[0] = vorsize[0];
m_VoronoiBoundaryOrigin[0] = vorsize[1];
}Проект ITK, язык C++
int itkPointSetToSpatialObjectDemonsRegistrationTest(....)
{
....
// Set its position
EllipseType::TransformType::OffsetType offset;
offset[0]=50;
offset[1]=50;
offset[1]=50;
....
}Чистый Copy-Paste.
Проект ReactOS, язык 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);
....
}По всей видимости, константа crBackgnd должна записываться в ячейку cols[2].
Проект Coin3D, язык 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))) {
....
}Дважды проверятся элемент массива size[1], а элемент size[2] не проверятся. Вот так и появляются странные артефакты на изображениях.
Проект OpenCV, язык C++
bool Jpeg2KDecoder::readHeader()
{
....
cmptlut[0] = ....
cmptlut[1] = ....
cmptlut[2] = ....
if( cmptlut[0] < 0 || cmptlut[1] < 0 || cmptlut[0] < 0 )
result = false;
....
}Прям чувствуется, как выражение cmptlut[0] < 0 было дважды размножено копированием, но поправили ноль только в одном месте.
Проект Visualization Toolkit (VTK), язык C++
void vtkImageStencilRaster::PrepareForNewData(....)
{
....
if (allocateExtent &&
allocateExtent[1] >= allocateExtent[1])
....
}Здесь и далее я не буду комментировать многие подобные ошибки. Что тут комментировать? Главное, разглядывая подобные фрагменты кода, прочувствовать, что хотя ошибка простая, это не значит, что она будет замечена программистом.
Проект Visualization Toolkit (VTK), язык 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;
....
}Здесь программист явно торопился побыстрее написать код. По-другому сложно объяснить, как он ошибся трижды. Элементы массива вычитаются сами из себя. В результате получается, что этот код эквивалентен:
inZPtr +=
(0)*inIncs[0] * data_type_size +
(0)*inIncs[1] * data_type_size +
(0)*inIncs[2] * data_type_size;Впрочем, этот код можно сократить ещё больше:
inZPtr += 0;Великолепно. В коде есть длинное серьёзно выглядящее выражение, которое, на самом деле, ничего не делает. Люблю такие случаи.
Проект Visualization Toolkit (VTK), язык C++
Аналогичный случай спешного написания кода.
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);
}
}Дважды повторяется сравнение newPos[2] != oldPos[2].
Проект 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);
}
....
}В условии должно проверяться, что встречены две косые черты, стоящие после двоеточия. Другими словами, ищется подстрока "://". Из-за опечатки, проверка «подслеповата» и готова второй косой чертой посчитать любой символ.
Проект 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]));
....
}Опечатка кроется вот здесь, в передаваемых в макрос аргументах:
IPP_MIN(m_cur.AcRate[2],m_cur.AcRate[2])Получается, что выбирается минимум из двух одинаковых значений. На самом деле, должно быть написано:
IPP_MIN(m_cur.AcRate[2],m_cur.AcRate[3])Кстати, этот код может продемонстрировать пользу стандартной библиотеки. Если написать так:
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));То код станет короче, и меньше подвержен ошибкам. Собственно, чем меньше однотипного кода, тем больше шансов, что он будет написан правильно.
Проект 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]);
....
}Правильное выражение:
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;
}Дублируется ряд действий по инициализации структуры. Те строчки, что помечены комментарием //2, можно удалить, и ничего не изменится. Я сомневался, включать ли этот фрагмент кода в статью. Здесь не совсем ошибка, и не совсем с индексами. Однако этот лишний код, скорее всего, появился именно из-за того, что программист запутался во всех этих членах класса и индексах 1, 2. Так что, думаю, этот фрагмент кода подходит для демонстрации, как легко путаться в числах.
Проект CMake, С
Рассматриваемый далее код написан не разработчиками CMake, а позаимствован. Судя по комментарию в начале файла, функция utf8_encode была написана Tim Kientzle ещё в 2007. С тех пор эта функция кочует из проекта в проект и много где встречается. Я не стал изучать вопрос первоисточника, так как это сейчас не принципиально. Раз этот код есть в проекте CMake, то и ошибка относится к 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;
....
}Как видите, есть какая-то путаница с индексами. Дважды происходит запись в элемент массива p[1]. Если изучить код по соседству, то становится понятно, что правильный код должен быть таким:
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;Примечание
Обратите внимание, что все ошибки, рассмотренные в этой главе, относятся к коду на языке C или C++. Нет кода на C# или Java!
Это очень интересно, я не ожидал подобного. На мой взгляд, рассмотренные опечатки никак не зависят от языка. И в следующих главах действительно появятся ошибки в коде на других языках. Думаю, это просто случайное совпадение. Анализатор PVS-Studio начал гораздо позже поддерживать языки C#/Java по сравнению с C/C++, и мы просто не успели накопить в базе соответствующее примеры ошибок.
Тем не менее, наблюдение всё равно интересное. Видимо, C и C++ программисты больше любят использовать числа 0, 1 и 2 при работе с массивами :).
Опечатки в именах
Это будет самый большой раздел. Людям очень легко запутаться в таких именах, как a1 и a2. Кажется, как тут можно запутаться? Можно. Легко. И сейчас читатель сможет в этом убедиться.
Проект Hive, язык 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;
}Функция сравнения compare принимает два объекта: o1 и o2. Но из-за опечатки далее используется только o2.
Что интересно, благодаря 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, язык C#
private void MergeParallelTransitions()
{
....
if (double.IsInfinity(transition1.Weight.Value) &&
double.IsInfinity(transition1.Weight.Value))
....
}Проект Doom 3, язык C++
uint AltOp::fixedLength()
{
uint l1 = exp1->fixedLength();
uint l2 = exp1->fixedLength();
if (l1 != l2 || l1 == ~0u)
return ~0;
return l1;
}Если кто-то сразу не заметил опечатку, то нужно посмотреть на строку, где инициализируется переменная l2. Следовало использовать exp2.
Проект Source Engine SDK, язык 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;
}
....
}Правильно:
nFPSThreshold1 = 60;
nFPSThreshold2 = 50;Проект Linux Kernel, язык C
Кстати, опечатки могут быть не только в именах переменных, но и в именах макросов. Сейчас будет несколько таких примеров.
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;
}
....
}Как видите, дважды используется маска с именем BIT1, что делает вторую проверку бессмысленной. Тело второго, помеченного комментарием, условного оператора никогда не будет выполнено.
Проект CMaNGOS, язык C++
void AttackedBy(Unit* pAttacker) override
{
....
DoScriptText(urand(0, 1) ?
SAY_BELNISTRASZ_AGGRO_1 :
SAY_BELNISTRASZ_AGGRO_1,
m_creature, pAttacker);
....
}В игре планировалось случайное поведение, но всегда выбирается одна и та же константа SAY_BELNISTRASZ_AGGRO_1.
Проект Vangers: 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;
}
....
}Судя по написанному рядом коду, правильный вариант должен быть таким:
ret = (lang)
? iJoystickStickSwitch2[vkey - VK_STICK_SWITCH_1]
: iJoystickStickSwitch1[vkey - VK_STICK_SWITCH_1];Проект RT-Thread, язык 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 — это операционная система с открытым исходным кодом в реальном времени для встроенных устройств. Здесь мы видим путаницу между FIFO 0 и FIFO 1. И где-то кто-то столкнётся с глючным устройством.
Ошибка здесь:
if (CAN_FIFO0 == fifo_number){
....
}else if(CAN_FIFO0 == fifo_number){Вторая проверка всегда даёт false. Правильно:
if (CAN_FIFO0 == fifo_number){
....
}else if(CAN_FIFO1 == fifo_number){Проект Hive, язык 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")) {
....
}Анализатор PVS-Studio указывает сразу на 2 ошибки:
- Строка, хранящаяся в colOrScalar1, не может одновременно равняться строкам «Col» и «Column»;
- Строка, хранящаяся в colOrScalar1, не может одновременно равняться строкам «Col» и «Scalar».
Явно присутствует путаница в именах переменных.
Проект Shareaza, язык 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()))
....
}Правильно:
pAttr1->GetValue().CompareNoCase(pAttr2->GetValue())Примечание
Давайте сделаем небольшую паузу. Есть опасение, что просматривая гору банальных ошибок, мы забудем, зачем мы это делаем.
Задача не в том, чтобы посмеяться над чужим кодом. Всё это не повод тыкать пальцем и приговаривать: «Ха-ха, ну надо же». Этот повод задуматься!
Публикации нашей команды призваны показать, что никто из нас не застрахован от ошибок. Ошибки, описываемые в статье, появляются в коде намного чаще, чем можно ожидать. Ещё важно, что вероятность запутаться в 0, 1, 2 почти не зависит от квалификации программиста.
Полезно осознать, что людям свойственно ошибаться. Без этого нельзя сделать следующий шаг в повышении качества и надёжности кода. Понимая, что все мы ошибаемся, люди начинают стараться выявить ошибки на самых ранних этапах, используя стандарты кодирования, обзоры кода, юнит-тесты, статические и динамические анализаторы. Это очень хорошо.
Зачем написаны и так понятные вещи? К сожалению, общаясь с большим количеством разработчиков, мы вынуждены констатировать, что не всегда это так уж всем понятно. У многих слишком завышена самооценка и они просто не допускают мысли, что способны совершать простые ошибки. Это грустно.
Если вы тимлид/менеджер, то приглашаю заодно ознакомиться вот с этой заметкой.
Проект Qt, язык 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())
....
}Правильно:
const Numeric *const num2 = o2.as<Numeric>();Проект Android, язык 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;
}Сразу две опечатки, из-за которых переменные pr2.mStretchMode и pr2.mFallbackMode сравниваются сами с собой.
Проект Boost, язык C++
point3D operator/(const point3D &p1, const point3D &p2)
{
return point3D(p1.x/p2.x, p1.y/p2.y, p1.z/p1.z);
}В самом конце опечатались и поделили переменную p1.z саму на себя.
Проект Clang, язык C++
bool haveSameType(QualType Ty1, QualType Ty2) {
return (Context.getCanonicalType(Ty1) ==
Context.getCanonicalType(Ty2) ||
(Ty2->isIntegerType() &&
Ty2->isIntegerType()));
}Да, да, подобные ошибки анализатор PVS-Studio находит и в компиляторах. Правильно:
(Ty1->isIntegerType() &&
Ty2->isIntegerType())Проект Clang, язык 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())) {
....
}Правильно:
(Op0I->hasOneUse() || Op1I->hasOneUse())Проект Qt, язык C++
inline bool qCompare(QImage const &t1, QImage const &t2, ....)
{
....
if (t1.width() != t2.width() || t2.height() != t2.height()) {
....
}Проект NCBI Genome Workbench, язык 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 {
.....
}Ошибка в самой первой проверке. Должно быть написано:
if (!s1.IsSet() && s2.IsSet()) {Проект NCBI Genome Workbench, язык 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");
}
....
}Была размножена первая строчка условия, но затем программист поспешил и забыл заменить loc1 на loc2.
Проект FlashDevelop, С#
public void SetPrices(....)
{
UInt32 a0 = _choice.GetPrice0();
UInt32 a1 = _choice.GetPrice1();
UInt32 b0 = a1 + _choice2.GetPrice0(); // <=
UInt32 b1 = a1 + _choice2.GetPrice1();
....
}Проект FreeCAD, язык 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);
};Независимо от условия, выполняется одно и то же действие. Казалось бы, такой простой случай. Как можно было скопировать строчку и не исправить её? Можно.
Проект LibreOffice, язык C++
class SVX_DLLPUBLIC SdrMarkView : public SdrSnapView
{
....
const Point& GetRef1() const { return maRef1; }
const Point& GetRef2() const { return maRef1; }
....
};Классическая Copy-Paste ошибка. Правильно:
const Point& GetRef2() const { return maRef2; }Проект LibreOffice, язык 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();
....
}И ещё одна классическая Copy-Paste ошибка :). В одном месте 1 на 2 поправили, а в другом забыли.
Проект LibreOffice, язык C++
XMLTransformerOOoEventMap_Impl::XMLTransformerOOoEventMap_Impl(
XMLTransformerEventMapEntry *pInit,
XMLTransformerEventMapEntry *pInit2 )
{
if( pInit )
AddMap( pInit );
if( pInit )
AddMap( pInit2 );
}Здесь не ошибка в замене 1 на 2, а просто не добавили 2 во второе условие.
Возможно, вы немного устали. Поэтому предлагаю заварить чай или кофе, и мы продолжим знакомство с миром чисел 0, 1 и 2.
Проект Geant4 software, язык 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];
}
....
}Надеюсь, вы воспользовались советом и отдохнули. Готовы теперь найти в этом коде ошибку?
Поздравляю читателей, которые заметили ошибку. Вы молодцы!
Кто поленился искать, тех я тоже понимаю. Обзор подобного кода очень утомителен и есть желание как-то побыстрее пойти проверять что-то более интересное. Вот здесь статические анализаторы отлично помогают, так как не устают.
Ошибка в том, что эти две проверки совпадают:
if (areacode & sC0Min1Max) {
} else if (areacode & sC0Min1Max) {Если изучить код, то становится понятно, что ошибочной является самая первая проверка. Правильно:
if (areacode & sC0Min1Min) {
} else if (areacode & sC0Max1Min) {
} else if (areacode & sC0Max1Max) {
} else if (areacode & sC0Min1Max) {Проект CryEngine V, язык 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, язык C++
void CGitStatusListCtrl::OnContextMenuList(....)
{
....
if( (!this->m_Rev1.IsEmpty()) ||
(!this->m_Rev1.IsEmpty()) )
....
}Проект Geant4 software, язык 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, С#
private bool MembersMatch(ISymbol member1, ISymbol member2)
{
....
if (member1.DeclaredAccessibility !=
member1.DeclaredAccessibility
|| member1.IsStatic != member1.IsStatic)
{
return false;
}
....
}Как видите, пока фрагменты кода идут без пояснений. Собственно, тут и пояснять нечего. Можно только повздыхать.
Проект Dolphin Emulator, язык C++
bool IRBuilder::maskedValueIsZero(InstLoc Op1, InstLoc Op2) const
{
return (~ComputeKnownZeroBits(Op1) &
~ComputeKnownZeroBits(Op1)) == 0;
}Проект RunAsAdmin Explorer Shim, язык 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, язык 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, язык 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, язык C++
Ok, теперь давайте чуть-чуть посложнее. Попробуйте ради интереса найти ошибку здесь:
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);
....
}Картинка, чтобы не увидеть ответ сразу, и была возможность подумать.
Правильно, вместо orig->y1 — orig->y1 должно быть написано orig->y1 — orig->y2.
Проект .NET Compiler Platform, язык С#
public void IndexerMemberRace()
{
....
for (int i = 0; i < 20; i++)
{
....
if (i % 2 == 0)
{
thread1.Start();
thread2.Start();
}
else
{
thread1.Start();
thread2.Start();
}
....
}
....
}Интересный случай. В целях тестирования требуется запускать потоки в различном порядке. Однако из-за опечатки потоки стартуют всегда одинаково, вследствие чего тест проверяет меньше чем должен.
Правильно:
if (i % 2 == 0)
{
thread1.Start();
thread2.Start();
}
else
{
thread2.Start();
thread1.Start();
}Проект Samba, язык С
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;
}Функция сравнения никогда не вернёт 1, так как условие i2->pid > i2->pid не имеет смысла.
Естественно, это обыкновенная опечатка, и на самом деле должно быть написано:
if (i1->pid > i2->pid) return 1;Проект ChakraCore, язык C++
Последний случай в этой главе. Ура!
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))
....
}Прочие ошибки
Теперь поговорим о менее многочисленных паттернах ошибок, связанных с использованием чисел 0, 1, 2.
Опечатки в условиях, где явно используется константа 0/1/2
Проект ROOT, язык 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) {
....
}Странно дважды сравнивать переменную fSummaryVrs с 0.
Проект .NET CoreCLR, язык С#
void PutIA64Imm22(UINT64 * pBundle, UINT32 slot, INT32 imm22)
{
if (slot == 0) // <=
{
....
}
else if (slot == 1)
{
....
}
else if (slot == 0) // <=
{
....
}
....
}Проект FFmpeg, язык 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(....);
....
}Индекс / имя
Ранее мы рассматривали случаи, когда неверен индекс или имя. А вот ситуация, когда сразу и не скажешь, как классифицировать ошибку. Этот пример можно было отнести как к одной, так и к другой главе. Поэтому я решил привести его отдельно.
Проект Mesa 3D Graphics Library, язык 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;
....
}Этот код можно поправить так:
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())И можно поправить так:
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())Лишний 0
Иногда 0 оказывается лишним и вредным. Из-за него число может превратиться в восьмеричное там, где это не нужно. Или портят форматную строку.
Названные ошибки на подходят для этой статьи, но я считаю, что про них стоит упомянуть. Не буду приводить в статье код с этими ошибками, но если интересно, то на них можно посмотреть здесь:
- V536 Be advised that the utilized constant value is represented by an octal form, примеры;
- V638 A terminal null is present inside a string. The '\0xNN' characters were encountered. Probably meant: '\xNN', примеры.
Забыли написать +1
Проект 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';
}
....
}Правильный вариант:
copiedPath[len] = '.';
copiedPath[len + 1] = '\0';Примечание. Ситуация, когда забыли добавить единицу, вовсе не редка. Я точно помню, что не раз встречал такие случаи. Однако, когда я захотел набрать подобных примеров для статьи, я нашёл только этот пример. Мне жаль, что я не могу напугать вас этими ошибками побольше. Прошу прощения.
Ошибки форматирования (C#)
Чаще всего функции для построения строк оперируют с небольшим количеством аргументов. Вот и получается, что чаще всего ошибки связаны с использованием {0}, {1} или {2}.
Проект Azure PowerShell, язык 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)))
{
....
}
....
}Опечатались и дважды написали {0}. В результате в строку будет дважды вставлено имя this.Name. А вот имя this.ResourceGroupName не попадёт в создаваемую строку.
Проект Mono, язык C#
void ReadEntropy ()
{
if (reader.IsEmptyElement)
throw new XmlException (
String.Format ("WS-Trust Entropy element is empty.{2}",
LineInfo ()));
....
}Это вообще что-то странное. Нужно вставить то, чего нет. Скорее всего, этот код подвергся неудачному рефакторингу и оказался сломан.
Проект Xenko, язык 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));
}Программист забыл, что нумерация начинается с {0}, а не с {1}. Правильный код:
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, язык C#
private void DumpAttributes(Symbol s)
{
....
Console.WriteLine("{0} {1} {2}", pa.ToString());
....
}Аргументов явно недостаточно.
Выводы и рекомендации
Мне пришлось продемонстрировать очень много примеров, чтобы показать, что опечатки, связанные с 0, 1 и 2, заслуживают отдельного внимания.
Если бы я просто сказал: «Легко перепутать o1 и o2», вы бы согласились, но не придали этому то значение, какое придадите сейчас, прочитав или хотя бы пролистав статью.
Теперь вы предупреждены, и это хорошо. Предупреждён — значит вооружён. Теперь вы будете более внимательны на обзорах кода и уделите дополнительное внимание переменным, в именах которых увидите 0, 1, 2.
Дать какие-то рекомендации по оформлению кода, чтобы избежать подобных ошибок, сложно. Как вы видели, ошибки встречаются даже в таком простом коде, где и оформлять-то нечего.
Поэтому я не стану призывать избегать 0, 1, 2 и давать переменным длинные имена. Если вместо чисел начать писать Firts/Second/Left/Right и так далее, то соблазн скопировать имя или выражение будет ещё больше. Возможно, такая рекомендация в конечном итоге вовсе не сократит, а увеличит количество ошибок.
Тем не менее, когда вы пишете много однотипного кода, по-прежнему актуальна рекомендация «табличного оформления кода». Табличное форматирование не гарантирует отсутствие опечаток, но позволяет их легче и быстрее заметить. См. главу N13 в мини-книге "Главный вопрос программирования, рефакторинга и всего такого".
Есть ещё одна хорошая новость. Все рассмотренные в этой статье ошибки найдены с помощью статического анализатора кода PVS-Studio. Соответственно, внедряя в процесс разработки инструменты статического анализа, вы сможете выявить многие опечатки на самом раннем этапе.
Спасибо за внимание. Надеюсь, вам было интересно и страшно. Желаю надёжного кода и поменьше ошибаться с 0, 1, 2, чтобы Фредди не пришёл к вам.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Zero, one, two, Freddy's coming for you.
