Хабрахабр

Chromium: опечатки

Опечатки

Предлагаем вашему вниманию цикл статей, посвященных рекомендациям по написанию качественного кода на примере ошибок, найденных в проекте Chromium. Это четвёртая часть, которая будет посвящена проблеме опечаток и написанию кода с помощью «Copy-Paste метода».

От опечаток в коде никто не застрахован. Опечатки можно встретить в коде самых квалифицированных программистов. Квалификация и опыт, к сожалению, не защищают от того, чтобы перепутать название переменной или забыть что-то написать. Поэтому опечатки были, есть и будут.
Появление дополнительных ошибок провоцирует методика написания кода с помощью Copy-Paste. К сожалению, копировать код — это эффективный метод, и ничего с этим не сделать. Намного быстрее скопировать несколько строк и что-то в них поправить, чем написать фрагмент кода заново. Я даже не буду стараться отговорить от этого метода, так как сам грешен и использую копирование кода. Неприятным побочным эффектом копирования является то, что в скопированном фрагменте кода забывают что-то изменить. Эти ошибки в каком-то смысле тоже являются опечатками: по невнимательности забыли что-то изменить в тексте или изменили неправильно.

Давайте посмотрим, какие опечатки я заметил в процессе разбора отчета, выданного PVS-Studio. Как я писал в вводной статье, отчёт был просмотрен достаточно бегло, поэтому могут быть и другие незамеченные мной ошибки. В этой статье речь пойдёт о самых глупых и простых ошибках. Но от того, что ошибки глупые, они не перестают быть ошибками.

Разыменование нулевого указателя

Согласно Common Weakness Enumeration ошибки разыменования нулевого указателя можно классифицировать как CWE-476.

Приведённые далее ошибки относятся к коду проекта Chromium.

class Display : ....
{ .... std::unique_ptr<FocusController> focus_controller_; .... } Display::~Display() { .... if (!focus_controller_) { focus_controller_->RemoveObserver(this); focus_controller_.reset(); } ....
}

Неправильно написано условие. Указатель разыменовывается, если он нулевой. Символ '!' здесь явно лишний.

Предупреждение PVS-Studio: V522 CWE-476 Dereferencing of the null pointer 'focus_controller_' might take place. display.cc 52

Следующая ошибка достойна носить звание «Классической классики».

void AppViewGuest::CreateWebContents(....) { .... if (!guest_extension || !guest_extension->is_platform_app() || !embedder_extension | !embedder_extension->is_platform_app()) { callback.Run(nullptr); return; } ....
}

Опечатка. Вместо оператора '||' случайно написали оператор '|'. В результате, указатель embedder_extension разыменовывается независимо от того, нулевой он или нет.

Примечание. Если статью читает кто-то, слабо знакомый с языком C++, то предлагаю ознакомиться со статьёй "Short-circuit evaluation", чтобы понять, в чём тут дело.

Предупреждение PVS-Studio: V522 CWE-476 Dereferencing of the null pointer 'embedder_extension' might take place. Check the bitwise operation. app_view_guest.cc 186

Следующая ошибка связана с тем, что код не дописан. Я думаю, это тоже можно рассматривать как опечатку. Должны были присвоить какое-то значение умному указателю, но забыли.

std::unique_ptr<base::ListValue>
NetworkingPrivateServiceClient::GetEnabledNetworkTypes() { std::unique_ptr<base::ListValue> network_list; network_list->AppendString(::onc::network_type::kWiFi); return network_list;
}

Умный указатель по умолчанию является нулевым. Раз этот указатель после создания не изменяется, то произойдёт разыменование нулевого указателя.

Предупреждение PVS-Studio: V522 CWE-476 Dereferencing of the null pointer 'network_list' might take place. networking_private_service_client.cc 351

Давайте теперь изучим какой-нибудь более сложный случай.

Response CSSAgent::getMatchedStylesForNode(int node_id, Maybe<CSS::CSSStyle>* inline_style)
{ UIElement* ui_element = dom_agent_->GetElementFromNodeId(node_id); *inline_style = GetStylesForUIElement(ui_element); if (!inline_style) return NodeNotFoundError(node_id); return Response::OK();
}

Предупреждение PVS-Studio: V595 CWE-476 The 'inline_style' pointer was utilized before it was verified against nullptr. Check lines: 142, 143. css_agent.cc 142

Указатель inline_style разыменовывается до проверки на равенство nullptr. Мне кажется, это следствие опечатки: здесь просто пропустили символ звёздочка '*'. В этом случае корректный код должен выглядеть так:

*inline_style = GetStylesForUIElement(ui_element);
if (!*inline_style) return NodeNotFoundError(node_id);

Впрочем, возможно хотели проверить именно указатель inline_style. В этом случае это не опечатка, а ошибка в логике функции. Тогда, чтобы исправить код, нужно переместить проверку выше, до вызова функции GetStylesForUIElement. Тогда функция должна быть такой:

Response CSSAgent::getMatchedStylesForNode(int node_id, Maybe<CSS::CSSStyle>* inline_style)
{ UIElement* ui_element = dom_agent_->GetElementFromNodeId(node_id); if (!inline_style) return NodeNotFoundError(node_id); *inline_style = GetStylesForUIElement(ui_element); return Response::OK();
}

У меня закончились ошибки разыменования нулевого указателя, относящиеся к коду Chromium, но есть ещё одна, замеченная мною в движке V8.

bool Object::IsSmi() const { return HAS_SMI_TAG(this); } bool IC::IsHandler(Object* object) { return (object->IsSmi() && (object != nullptr)) || object->IsDataHandler() || object->IsWeakCell() || object->IsCode();
}

Указатель object вначале разыменовывается, а уже потом проверяется на равенство nullptr. Да и вообще выражение выглядит как-то подозрительно. Очень похоже на опечатку при поспешном программировании: сначала написали object->IsSmi(), потом вспомнили, что надо проверить указатель object на равенство nullptr и добавили проверку. А подумать забыли :).

Анализатор PVS-Studio выдаёт здесь сразу два предупреждения:

  • V522 CWE-628 Dereferencing of the null pointer 'object' might take place. The null pointer is passed into 'IsHandler' function. Inspect the first argument. Check lines: 'ic-inl.h:44', 'stub-cache.cc:19'. ic-inl.h 44
  • V713 CWE-476 The pointer object was utilized in the logical expression before it was verified against nullptr in the same logical expression. ic-inl.h 44

Copy-Paste

Невозможно классифицировать ошибки, допущенные из-за Copy-Paste, согласно Common Weakness Enumeration. Нет такого дефекта, как «Copy-Paste» :). Разные опечатки будут приводить к дефектам разного типа. Далее я покажу ошибки, которые можно классифицировать как:

  • CWE-563: Assignment to Variable without Use
  • CWE-570: Expression is Always False
  • CWE-571: Expression is Always True
  • CWE-682: Incorrect Calculation
  • CWE-691: Insufficient Control Flow Management

Начнём вновь с кода, относящегося непосредственно к проекту Chromium.

void ProfileSyncService::OnEngineInitialized(....)
{ .... std::string signin_scoped_device_id; if (IsLocalSyncEnabled()) { signin_scoped_device_id = "local_device"; } else { SigninClient* signin_client = ....; DCHECK(signin_client); std::string signin_scoped_device_id = // <= signin_client->GetSigninScopedDeviceId(); } ....
}

Я прямо чувствую, как программисту было лень набирать вновь имя переменной signin_scoped_device_id. И он решил скопировать это имя. Однако случайно, вместе с именем, он скопировал и тип std::string:

std::string signin_scoped_device_id

Следствием лени стало то, что возвращенное функцией GetSigninScopedDeviceId значение будет помещено во временную переменную, которая тут же будет уничтожена.

Предупреждение PVS_Studio: V561 CWE-563 It's probably better to assign value to 'signin_scoped_device_id' variable than to declare it anew. Previous declaration: profile_sync_service.cc, line 900. profile_sync_service.cc 906

Следующую ошибку я встретил в движке V8, который используется в Chromium.

static LinkageLocation ForSavedCallerReturnAddress() { return ForCalleeFrameSlot( (StandardFrameConstants::kCallerPCOffset - StandardFrameConstants::kCallerPCOffset) / kPointerSize, MachineType::Pointer());
}

Скорее всего программист скопировал StandardFrameConstants::kCallerPCOffset и хотел изменить имя константы, но забыл это сделать. Поэтому в коде константа вычитается сама из себя, в результате чего получается 0. После чего этот 0 делится на kPointerSize, но это уже не имеет никакого значения. Все равно получится 0.

Предупреждение PVS-Studio: V501 There are identical sub-expressions 'StandardFrameConstants::kCallerPCOffset' to the left and to the right of the '-' operator. linkage.h 66

Ещё один подозрительный фрагмент кода, замеченный в V8:

void JSObject::JSObjectShortPrint(StringStream* accumulator) { .... accumulator->Add(global_object ? "<RemoteObject>" : "<RemoteObject>"); ....
}

Предупреждение PVS-Studio: V583 CWE-783 The '?:' operator, regardless of its conditional expression, always returns one and the same value: "". objects.cc 2993

Теперь заглянем в проект PDFium.

inline bool FXSYS_iswalpha(wchar_t wch) { return FXSYS_isupper(wch) || FXSYS_islower(wch);
} bool CPDF_TextPage::IsHyphen(wchar_t curChar) const { WideStringView curText = m_TempTextBuf.AsStringView(); .... auto iter = curText.rbegin(); .... if ((iter + 1) != curText.rend()) { iter++; if (FXSYS_iswalpha(*iter) && FXSYS_iswalpha(*iter)) // <= return true; } ....
}

Скопировали FXSYS_iswalpha(*iter). И… И что-то забыли изменить во второй части условия.

Предупреждение PVS-Studio: V501 CWE-571 There are identical sub-expressions 'FXSYS_iswalpha(* iter)' to the left and to the right of the '&&' operator. cpdf_textpage.cpp 1218

Схожую ошибку при написании выражения можно встретить в библиотеке Protocol buffers.

bool IsMap(const google::protobuf::Field& field, const google::protobuf::Type& type) { return field.cardinality() == google::protobuf::Field_Cardinality_CARDINALITY_REPEATED && (GetBoolOptionOrDefault(type.options(), "map_entry", false) || GetBoolOptionOrDefault(type.options(), "google.protobuf.MessageOptions.map_entry", false) || // <= GetBoolOptionOrDefault(type.options(), "google.protobuf.MessageOptions.map_entry", false)); // <=
}

Код явно написан с помощью Copy-Paste. Никто не будет повторно набирать такую длинную строчку :).

Предупреждение PVS-Studio: V501 CWE-570 There are identical sub-expressions to the left and to the right of the '||' operator. utility.cc 351

Кстати, рядышком есть ещё одна такая же ошибка: V501 CWE-570 There are identical sub-expressions to the left and to the right of the '||' operator. utility.cc 360

В следующем фрагменте кода из библиотеки SwiftShader нас ждёт мой любимый "эффект последней строки". Красивый баг! Люблю такие.

void TextureCubeMap::updateBorders(int level)
{ egl::Image *posX = image[CubeFaceIndex(..._POSITIVE_X)][level]; egl::Image *negX = image[CubeFaceIndex(..._NEGATIVE_X)][level]; egl::Image *posY = image[CubeFaceIndex(..._POSITIVE_Y)][level]; egl::Image *negY = image[CubeFaceIndex(..._NEGATIVE_Y)][level]; egl::Image *posZ = image[CubeFaceIndex(..._POSITIVE_Z)][level]; egl::Image *negZ = image[CubeFaceIndex(..._NEGATIVE_Z)][level]; .... if(!posX->hasDirtyContents() || !posY->hasDirtyContents() || !posZ->hasDirtyContents() || !negX->hasDirtyContents() || !negY->hasDirtyContents() || // <= !negY->hasDirtyContents()) // <= { return; } ....
}

В самом конце условия следовало использовать не указатель negY, а указатель negZ. Явно имеем дело с Copy-Paste строки кода и потерей внимания в самом конце.

Предупреждение PVS-Studio: V501 CWE-570 There are identical sub-expressions '!negY->hasDirtyContents()' to the left and to the right of the '||' operator. texture.cpp 1268

Движок WebKit тоже порадовал красивым багом:

bool IsValid(....) const final { OptionalRotation inherited_rotation = GetRotation(*state.ParentStyle()); if (inherited_rotation_.IsNone() || inherited_rotation.IsNone()) return inherited_rotation.IsNone() == inherited_rotation.IsNone(); ....
}

Предупреждение PVS-Studio: V501 CWE-571 There are identical sub-expressions 'inherited_rotation.IsNone()' to the left and to the right of the '==' operator. cssrotateinterpolationtype.cpp 166

Скопировали inherited_rotation.IsNone() и забыли добавить символ подчёркивания '_'. Правильный вариант:

return inherited_rotation_.IsNone() == inherited_rotation.IsNone();

Вновь заглянем в библиотеку Protocol buffers.

void SetPrimitiveVariables(...., std::map<string, string>* variables) { .... (*variables)["set_has_field_bit_message"] = ""; (*variables)["set_has_field_bit_message"] = ""; (*variables)["clear_has_field_bit_message"] = ""; ....
}

Пояснения излишни. Copy-Paste в чистом виде. Предупреждение PVS-Studio: V519 CWE-563 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 149, 150. java_primitive_field_lite.cc 150

Продолжим. Программисты должны знать, сколь коварен Copy-Paste. Читайте и ужасайтесь. Перед нами функция, взятая из WebRTC.

size_t WebRtcSpl_FilterAR(....)
{ .... for (i = 0; i < state_length - x_length; i++) { state[i] = state[i + x_length]; state_low[i] = state_low[i + x_length]; } for (i = 0; i < x_length; i++) { state[state_length - x_length + i] = filtered[i]; state[state_length - x_length + i] = filtered_low[i]; // <= } ....
}

Да, вновь последствия Copy-Paste. Скопировали строку:

state[state_length - x_length + i] = filtered[i];

Заменили в ней filtered на filtered_low. А вот заменить state на state_low забыли. В результате часть элементов массива state_low остаются неинициализированными.

Вы устали читать? Представьте, как я устал это писать! Давайте передохнём и выпьем кофе.

Picture 3

ПАУЗА.

Надеюсь вы восстановили силы и готовы продолжить наслаждаться 50-тью оттенками Copy-Paste. Вот что можно увидеть в библиотеке PDFium.

bool CPWL_EditImpl::Backspace(bool bAddUndo, bool bPaint) { .... if (m_wpCaret.nSecIndex != m_wpOldCaret.nSecIndex) { AddEditUndoItem(pdfium::MakeUnique<CFXEU_Backspace>( this, m_wpOldCaret, m_wpCaret, word.Word, word.nCharset)); } else { AddEditUndoItem(pdfium::MakeUnique<CFXEU_Backspace>( this, m_wpOldCaret, m_wpCaret, word.Word, word.nCharset)); } ....
}

Независимо от условия выполняется одно и тоже действие. Скорее всего, здесь неудачный Copy-Paste. Программист скопировал фрагмент, потом отвлёкся и забыл поправить else-ветку.

Предупреждения PVS-Studio: V523 CWE-691 The 'then' statement is equivalent to the 'else' statement. cpwl_edit_impl.cpp 1580

Есть ещё три таких же места. Я пожалею читателя и приведу только сообщения анализатора:

  • V523 CWE-691 The 'then' statement is equivalent to the 'else' statement. cpwl_edit_impl.cpp 1616
  • V523 CWE-691 The 'then' statement is equivalent to the 'else' statement. cpdf_formfield.cpp 172
  • V523 CWE-691 The 'then' statement is equivalent to the 'else' statement. cjs_field.cpp 2323

Библиотека Skia.

bool SkPathRef::isValid() const { .... if (nullptr == fPoints && 0 != fFreeSpace) { return false; } if (nullptr == fPoints && 0 != fFreeSpace) { return false; } ....
}

Два раза выполняется одинаковая проверка. Вторая проверка лишняя. Или забыли проверить что-то другое. Анализатор сигнализирует о подозрительности этого кода сразу двумя предупреждениями:

  • V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 758, 761. skpathref.cpp 761
  • V649 CWE-561 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 758, 761. skpathref.cpp 761

И ещё одна ошибка в библиотеке Skia.

static inline bool can_blit_framebuffer_for_copy_surface( const GrSurface* dst, GrSurfaceOrigin dstOrigin, const GrSurface* src, GrSurfaceOrigin srcOrigin, ....)
{ .... const GrGLTexture* dstTex = static_cast<const GrGLTexture*>(dst->asTexture()); const GrGLTexture* srcTex = static_cast<const GrGLTexture*>(dst->asTexture()); // <= const GrRenderTarget* dstRT = dst->asRenderTarget(); const GrRenderTarget* srcRT = src->asRenderTarget(); if (dstTex && dstTex->target() != GR_GL_TEXTURE_2D) { return false; } if (srcTex && srcTex->target() != GR_GL_TEXTURE_2D) { return false; } ....
}

Предупреждение PVS-Studio: V656 Variables 'dstTex', 'srcTex' are initialized through the call to the same function. It's probably an error or un-optimized code. Check lines: 3312, 3313. grglgpu.cpp 3313

После Copy-Paste забыли заменить dst на src. Должно быть написано:

const GrGLTexture* srcTex = static_cast<const GrGLTexture*>(src->asTexture());

Библиотека HarfBuzz.

inline int get_kerning (hb_codepoint_t left, hb_codepoint_t right, const char *end) const
{ unsigned int l = (this+leftClassTable).get_class (left); unsigned int r = (this+leftClassTable).get_class (left); // <= unsigned int offset = l * rowWidth + r * sizeof (FWORD); ....
}

Предупреждение PVS-Studio: V751 Parameter 'right' is not used inside function body. hb-ot-kern-table.hh 115

Красивая ошибка. Скопировали строчку, но забыли два раза поменять left на right. Должно быть:

unsigned int l = (this+leftClassTable).get_class (left);
unsigned int r = (this+rightClassTable).get_class (right);

Библиотека SwiftShader. Уверен, эти однотипные фрагменты кода писались с использование Copy-Paste:

class ELFObjectWriter { .... ELFStringTableSection *ShStrTab; ELFSymbolTableSection *SymTab; ELFStringTableSection *StrTab; ....
}; void ELFObjectWriter::assignSectionNumbersInfo( SectionList &AllSections)
{ .... ShStrTab->setNumber(CurSectionNumber++); ShStrTab->setNameStrIndex(ShStrTab->getIndex(ShStrTab->getName())); AllSections.push_back(ShStrTab); SymTab->setNumber(CurSectionNumber++); SymTab->setNameStrIndex(ShStrTab->getIndex(SymTab->getName())); AllSections.push_back(SymTab); StrTab->setNumber(CurSectionNumber++); StrTab->setNameStrIndex(ShStrTab->getIndex(StrTab->getName())); AllSections.push_back(StrTab); ....
}

Программист был невнимателен. Во втором блоке текста он забыл заменить hStrTab->getIndex на на SymTab->getIndex, а в третьем блоке не заменил hStrTab->getIndex на StrTab->getIndex.

Предупреждение PVS-Studio: V778 CWE-682 Two similar code fragments were found. Perhaps, this is a typo and 'SymTab' variable should be used instead of 'ShStrTab'. iceelfobjectwriter.cpp 194

Следующая ошибка связана с неправильным вычислением размера прямоугольника в библиотеке WebKit. Код «вырви глаз». Слабо заметить ошибку?

void NGFragmentBuilder::ComputeInlineContainerFragments(....)
{ .... value.start_fragment_union_rect.size.width = std::max(descendant.offset_to_container_box.left + descendant.fragment->Size().width - value.start_fragment_union_rect.offset.left, value.start_fragment_union_rect.size.width); value.start_fragment_union_rect.size.height = std::max(descendant.offset_to_container_box.top + descendant.fragment->Size().height - value.start_fragment_union_rect.offset.top, value.start_fragment_union_rect.size.width); ....
}

В самом конце забыли заменить в скопированном блоке width на height.

Предупреждение PVS-Studio: V778 CWE-682 Two similar code fragments were found. Perhaps, this is a typo and 'height' variable should be used instead of 'width'. ng_fragment_builder.cc 326

Уфф… Заканчиваем. Последний фрагмент кода в этом разделе взят из библиотеки PDFium.

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

Повторяющийся блок присваиваний. Предупреждение PVS-Studio: V760 Two identical blocks of text were found. The second block begins from line 420. fx_codec_jpx_opj.cpp 416

Нет, я вас обманул. Встретился ещё один Copy-Paste из PDFium. Пришлось добавить в статью.

void Transform(int x, int y, int* x1, int* y1, int* res_x, int* res_y) const
{ .... if (*res_x < 0 && *res_x > -kBase) *res_x = kBase + *res_x; if (*res_y < 0 && *res_x > -kBase) *res_y = kBase + *res_y; }
}

Предупреждение PVS-Studio: V778 CWE-682 Two similar code fragments were found. Perhaps, this is a typo and 'res_y' variable should be used instead of 'res_x'. cfx_imagetransformer.cpp 201

Была скопирована строчка:

if (*res_x < 0 && *res_x > -kBase)

Одно имя переменной res_x заменили на res_y, а второе забыли. В результате в функции отсутствует проверка условия *res_y > -kBase.

Другие опечатки

Если опечатки типа «нулевые указатели» и «Copy-Paste» выделились как-то сами собой, то оставшиеся ошибки достаточно разнородны. Я не стал пытаться их как-то классифицировать и просто собрал в этом разделе статьи.

Начнем с фрагмента кода, относящегося к проекту Chromium. Я не уверен, что это ошибка. Однако это место явно стоит проверить разработчикам.

namespace cellular_apn { const char kAccessPointName[] = "AccessPointName"; const char kName[] = "Name"; const char kUsername[] = "Username"; const char kPassword[] = "Password"; const char kLocalizedName[] = "LocalizedName"; const char kLanguage[] = "LocalizedName";
}

Подозрительно то, что константы kLocalizedName и kLanguage содержат в себе одну и ту же строку. Рискну предположить, что должно быть написано вот так:

const char kLanguage[] = "Language";

Впрочем, я не уверен.

Анализатор PVS-Studio выдаёт здесь предупреждение: V691 Empirical analysis. It is possible that a typo is present inside the string literal: «LocalizedName». The 'Localized' word is suspicious. onc_constants.cc 162

Следующая ошибка в библиотеке Skia просто прекрасна и отсылает нас к статье "Зло живёт в функциях сравнения".

inline bool operator==(const SkPDFCanon::BitmapGlyphKey& u, const SkPDFCanon::BitmapGlyphKey& v) { return memcmp(&u, &u, sizeof(SkPDFCanon::BitmapGlyphKey)) == 0;
}

Из-за опечатки объект u сравнивается сам с собой. Получается, что operator == считает любые два объекта одинаковыми.

Предупреждение PVS-Studio: V549 CWE-688 The first argument of 'memcmp' function is equal to the second argument. skpdfcanon.h 67

Следующая опечатка в библиотеке Skia приводит к тому, что функция распечатывает не все элементы массива, а многократно выводит информацию о первом элементе. Впрочем, ошибка нестрашная, так как функция предназначена для отладочных целей.

SkString dumpInfo() const override { SkString str; str.appendf("# combined: %d\n", fRects.count()); for (int i = 0; i < fRects.count(); ++i) { const RectInfo& geo = fRects[0]; str.appendf("%d: Color: 0x%08x, " "Rect [L: %.2f, T: %.2f, R: %.2f, B: %.2f]\n", i, geo.fColor, geo.fRect.fLeft, geo.fRect.fTop, geo.fRect.fRight, geo.fRect.fBottom); } str += fHelper.dumpInfo(); str += INHERITED::dumpInfo(); return str;
}

Вместо fRects[0] должно быть написано fRects[i]. Предупреждение PVS-Studio: V767 Suspicious access to element of 'fRects' array by a constant index inside a loop. grnonaafillrectop.cpp 276

Опечатка в проекте SwiftShader приводит к тому, что макрос assert проверяет не всё, что требуется.

static Value *createArithmetic(Ice::InstArithmetic::OpKind op, Value *lhs, Value *rhs)
{ assert(lhs->getType() == rhs->getType() || (llvm::isa<Ice::Constant>(rhs) && (op == Ice::InstArithmetic::Shl || Ice::InstArithmetic::Lshr || Ice::InstArithmetic::Ashr))); ....
}

Два раза забыли написать «op ==». В результате, частью условия являются константы Ice::InstArithmetic::Lshr и Ice::InstArithmetic::Ashr, которые ни с чем не сравниваются. Это явная ошибка, из-за которой часть условия всегда истинно.

Здесь на самом деле должно быть написано:

assert(lhs->getType() == rhs->getType() || (llvm::isa<Ice::Constant>(rhs) && (op == Ice::InstArithmetic::Shl || op == Ice::InstArithmetic::Lshr || op == Ice::InstArithmetic::Ashr)));

Анализатор PVS-Studio выдаёт два предупреждения:

  • V768 CWE-571 The enumeration constant 'Lshr' is used as a variable of a Boolean-type. subzeroreactor.cpp 712
  • V768 CWE-571 The enumeration constant 'Ashr' is used as a variable of a Boolean-type. subzeroreactor.cpp 712

Кстати, ещё встречаются такие фрагменты кода, которые сами по себе не являются ошибкой или опечаткой. Но они провоцируют опечатки в дальнейшем. Например, это неудачные имена глобальных переменных.

Одну из таких переменных мы можем наблюдать в библиотеке Yasm:

static int i; /* The t_type of tokval */

Предупреждение PVS-Studio: V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'i' variable. nasm-eval.c 29

Да, это ещё не ошибка. Но очень легко где-то забыть объявить локальную переменную i и воспользоваться глобальной. Причем код скомпилируется, но как после этого будет работать программа — большая загадка. В общем, лучше давать глобальным переменным какие-то более уникальные имена.

Надеюсь, я смог донести всю серьезность ошибок, которые возникают из-за опечаток!

Рисунок 1

Напоследок, предлагаю познакомиться с красивой опечаткой в библиотеке Protocol buffers, которую я описал в отдельной заметке — "31 февраля".

Рекомендации

Прошу прощения, в этот раз я вас подведу с разделом «рекомендации». Очень сложно дать какие-то чёткие универсальные советы, чтобы избегать ошибок, рассмотренных в статье. Человеку свойственно делать такие ошибки, и всё тут.

Тем не менее, я всё-таки попробую хоть как-то помочь, чтобы уменьшить количество опечаток в программах.

  1. Используйте «табличное оформление» сложных условий. Я подробно описал эту технологию в мини-книге "Главный вопрос программирования, рефакторинга и всего такого". Смотрите совет N13 — Выравнивайте однотипный код «таблицей». Кстати, в этом году я планирую написать расширенный вариант этой книги. В неё будет входить уже не 42, а 50 рекомендаций. Плюс некоторые старые главы нуждаются в обновлении и доработке.
  2. Занимаясь Copy-Paste, сосредоточьтесь в конце работы. Это связано с "эффектом последней строки", когда в конце написания однотипных блоков текста человек расслабляется и допускает ошибки. Если хочется почитать что-то более научное на эту тему, то предлагаю статью "Объяснение эффекта последней строки".
  3. Будьте аккуратны при написании функций, сравнивающих объекты. Эти функции обманчиво просты и провоцируют появление большого количества опечаток. Подробности: "Зло живёт в функциях сравнения".
  4. Если код тяжело читать, то и заметить в нём ошибку сложно. Поэтому старайтесь писать код как можно более понятным и легко читаемым. К сожалению, это сложно формально сформулировать, тем более кратко. Этой теме посвящены многие замечательные книги, например, такая как «Совершенный код», написанная Стивом Макконнелом. С практической точки зрения рекомендую уделять внимание стандарту кодирования в вашей компании и не лениться пополнять его хорошими практиками, о которых где-то узнали. И вообще, C++ быстро развивается, поэтому есть смысл регулярно проводить аудит стандарта и обновлять его.
  5. Регулярно используйте статический анализатор кода PVS-Studio. В конце концов, все описанные в этой статье опечатки были найдены именно с помощью PVS-Studio. Скачать и попробовать анализатор можно здесь.

Спасибо всем за внимание, но я ещё не прощаюсь. Скоро будет очередная статья.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Chromium: Typos.

Теги
Показать больше

Похожие статьи

Кнопка «Наверх»
Закрыть