Предлагаем вашему вниманию цикл статей, посвященных рекомендациям по написанию качественного кода на примере ошибок, найденных в проекте 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: "<RemoteObject>". 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 остаются неинициализированными.

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

ПАУЗА.

Надеюсь вы восстановили силы и готовы продолжить наслаждаться 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 и воспользоваться глобальной. Причем код скомпилируется, но как после этого будет работать программа — большая загадка. В общем, лучше давать глобальным переменным какие-то более уникальные имена.

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

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

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

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

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

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

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

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