Этот пост продолжает серию статей, которые вполне можно назвать «ужастиками для разработчиков». На этот раз мы также коснемся типичных опечаток, связанных с использованием чисел 0, 1, 2. Язык, на котором вы пишете, не имеет большого значения: это может быть C, C++, C# или Java. Если вы используете константы 0, 1, 2 или имена переменных содержат эти числа, скорее всего, Фредди придет к вам ночью. Давай, читай и не говори, что мы тебя не предупреждали.
Введение
Продолжаю цикл статей о замеченных закономерностях того, как люди совершают ошибки. Предыдущие сообщения:
На этот раз закономерность заметил не я, а мой коллега Святослав Размыслов. Он заметил, что в своих статьях постоянно описывал задачи, связанные с переменными с цифрами 1 и 2 в именах. Святослав предложил мне более подробно изучить этот вопрос. В итоге приложенные усилия оказались очень плодотворными. Выяснилось, что в нашей коллекции ошибок много фрагментов кода, ошибочных из-за того, что люди запутались в 0, 1, 2 индексах или именах переменных, содержащих такие числа. Выявлена новая интересная закономерность, о которой пойдет речь ниже. Я благодарен Святославу за подсказку заглянуть в эту тему и поэтому посвящаю эту статью ему.
Святослав Размыслов, менеджер, внимательный багхантер и просто талантливый человек.
Какова цель этой статьи? Чтобы показать, как легко всем нам ошибаться и делать опечатки. Предупрежденные разработчики — более внимательные разработчики. Особенно во время проверки кода, когда они фокусируются на этих злополучных 0, 1, 2. Разработчики также смогут оценить вклад статических анализаторов кода, которые помогают выявлять такие ошибки. Дело не в рекламе PVS-Studio (ну, в какой-то степени так и есть :). До сих пор многие разработчики считают статический анализ излишним, предпочитая сосредоточиться на собственной точности и проверках кода. К сожалению, попытки писать чистый код похвальны, но этого недостаточно. Данная статья еще раз убедительно продемонстрирует это.
Никто не застрахован от ошибок. Ниже вы увидите эпические ляпы даже в таких известных проектах, как Qt, Clang, Hive, LibreOffice, Linux Kernel, .NET Compiler Platform, ядро XNU, Mozilla Firefox. Кстати, это не какие-то экзотические редкие ошибки, а самые распространенные. Все еще недостаточно убедительно? Тогда вперед!
«Говорить дешево. Покажи мне ошибки!»
© переделанная цитата Линуса Торвальдса.
Опечатки в константах при индексации массивов
Обычно в наших статьях мы приводим предупреждения, которые помогли найти те или иные ошибки. В этот раз я опущу эти предупреждения, так как и без них ошибки все равно будут очевидны и понятны. Несмотря на то, что эти ошибки проявляются в коротком фрагменте кода, они отлично прячутся в коде проектов.
Начнем с путаницы с числовыми литералами, используемыми для индексации массивов. Несмотря на банальность этих ошибок, их много и их можно найти в проектах, которые намного больше, чем студенческие лабораторные исследования.
Проект ядра XNU, 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;
}
....
};Да, даже в таком коротком конструкторе можно найти способ выйти за пределы массива при его инициализации.
Движок Годо, C++
Array PhysicsDirectSpaceState::_cast_motion(....)
{
....
Array ret(true);
ret.resize(2);
ret[0]=closest_safe;
ret[0]=closest_unsafe;
return ret;
}Комментарии не нужны.
Звездочка, 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; .... }
При записи подобных блоков ошибка обычно в последнем. Все вышеперечисленные случаи были такими, кроме последнего. Здесь опечатка в необычном месте, а именно в первом блоке. Трудно сказать, почему так произошло. Я просто оставлю картинку единорога, пожимающего плечами:
Открытая технология CASCADE, 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;Транспротеомный конвейер, 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 (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;
....
}Копировать-вставить в лучшем виде.
Проект 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 было скопировано дважды, но 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] повторяется дважды.
АДАПТИВНАЯ коммуникационная среда (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, 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, C
Следующий код написан не разработчиками CMake, а заимствован. Как говорится в комментарии в начале файла, функция utf8_encode была написана Тимом Кинцле еще в 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, 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.
Проект Вангера: 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 сравниваются сами с собой.
Ускоренный проект, 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, C#
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; }
....
};Классическая ошибка копирования-вставки. Правильная версия:
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, 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, 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, C#
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, 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++
Хорошо, теперь перейдем к более сложным случаям. Попробуйте найти ошибку здесь, ради интереса:
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-›y2 нужно писать вместо orig-›y1 — orig-›y1.
Проект платформы компилятора .NET, C#
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, 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; }
Функция сравнения никогда не вернет 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, C#
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 Обратите внимание, что используемое постоянное значение представлено в восьмеричной форме, примеры;
- V638 Внутри строки присутствует терминальный ноль. Обнаружены символы ‘\0xNN’. Вероятно имелось в виду: ‘\xNN’, примеры.
Забыл написать +1
Операционная система Haiku, 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';Примечание. Случай, когда забывают прибавить 1, не редкость. Я точно помню, что видел такие случаи довольно часто. Однако когда я захотел собрать такие примеры для статьи, то нашел только этот пример. Извините, я не могу напугать вас большими ошибками. Я действительно сожалею.
Ошибки форматирования (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, то оно не попадет в созданную строку.
Монопроект, 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, C#
private void DumpAttributes(Symbol s)
{
....
Console.WriteLine("{0} {1} {2}", pa.ToString());
....
}Аргументов явно недостаточно.
Заключение и рекомендации
Пришлось привести довольно много примеров, чтобы показать, что опечатки, связанные с 0, 1 и 2, заслуживают особого внимания.
Если бы я просто сказал: «Легко спутать о1 и о2», вы бы согласились, но не уделили бы этому особого внимания, как уделяете сейчас, прочитав или хотя бы просмотрев статью.
Теперь вы предупреждены, и это хорошо. Предупрежден - значит вооружен. Отныне вы будете более внимательны при просмотре кода и будете уделять особое внимание переменным с 0, 1, 2 в именах.
Сложно дать определенные рекомендации по форматированию кода, чтобы избежать вышеуказанных ошибок. Как вы видели, ошибки случаются даже в таком простом коде, где фактически нечего форматировать.
Поэтому я не буду называть, чтобы избежать 0, 1, 2 и давать переменным длинные имена. Если вместо цифр вы начнете писать First/Second/Left/Right, то соблазн скопировать имя или выражение будет еще больше. Возможно, эта рекомендация в итоге не уменьшит, а увеличит количество ошибок.
Однако, когда вы пишете много однотипного кода, рекомендация табличного форматирования кода по-прежнему актуальна. Форматирование таблицы не гарантирует отсутствие опечаток, но помогает их легче и быстрее заметить. Подробнее см. главу 13 из мини-книги Главный вопрос программирования, рефакторинга и всего остального.
Есть еще одна хорошая новость. Все ошибки, рассмотренные в этой статье, обнаруживаются статическим анализатором кода PVS-Studio. Соответственно, внедрив в процесс разработки инструменты статического анализа, вы сможете выявить многие опечатки на самом раннем этапе.
Спасибо за внимание. Надеюсь, вам было интересно и страшно. Желаю вам надежного кода и поменьше ошибок с 0, 1, 2, чтобы Фредди к вам не приходил.
