Почему обзоры кода — это хорошо, но недостаточно

Андрей Карпов
Статей: 548

Обзоры кода однозначно нужны и полезны. Это возможность передать знания, обучение, контроль выполнения задачи, улучшение качества и оформления кода, исправление ошибок. Причем можно замечать высокоуровневые ошибки, связанные с используемой архитектурой и алгоритмами. В общем всё хорошо, но люди быстро устают. Поэтому статический анализ великолепно дополняет обзоры и помогает выявлять разнообразнейшие неприметные на глаз ошибки и опечатки. Рассмотрим хороший пример на эту тему.

https://import.viva64.com/docx/blog/0761_Why_Code_Reviews_Are_Good_But_Not_Enough_ru/image1.png

Попробуйте найти ошибку в коде функции, взятой из библиотеки structopt:

static inline bool is_valid_number(const std::string &input) {
  if (is_binary_notation(input) ||
      is_hex_notation(input) ||
      is_octal_notation(input)) {
    return true;
  }

  if (input.empty()) {
    return false;
  }

  std::size_t i = 0, j = input.length() - 1;

  // Handling whitespaces
  while (i < input.length() && input[i] == ' ')
    i++;
  while (input[j] == ' ')
    j--;

  if (i > j)
    return false;

  // if string is of length 1 and the only
  // character is not a digit
  if (i == j && !(input[i] >= '0' && input[i] <= '9'))
    return false;

  // If the 1st char is not '+', '-', '.' or digit
  if (input[i] != '.' && input[i] != '+' && input[i] != '-' &&
      !(input[i] >= '0' && input[i] <= '9'))
    return false;

  // To check if a '.' or 'e' is found in given
  // string. We use this flag to make sure that
  // either of them appear only once.
  bool dot_or_exp = false;

  for (; i <= j; i++) {
    // If any of the char does not belong to
    // {digit, +, -, ., e}
    if (input[i] != 'e' && input[i] != '.' &&
        input[i] != '+' && input[i] != '-' &&
        !(input[i] >= '0' && input[i] <= '9'))
      return false;

    if (input[i] == '.') {
      // checks if the char 'e' has already
      // occurred before '.' If yes, return false;.
      if (dot_or_exp == true)
        return false;

      // If '.' is the last character.
      if (i + 1 > input.length())
        return false;

      // if '.' is not followed by a digit.
      if (!(input[i + 1] >= '0' && input[i + 1] <= '9'))
        return false;
    }

    else if (input[i] == 'e') {
      // set dot_or_exp = 1 when e is encountered.
      dot_or_exp = true;

      // if there is no digit before 'e'.
      if (!(input[i - 1] >= '0' && input[i - 1] <= '9'))
        return false;

      // If 'e' is the last Character
      if (i + 1 > input.length())
        return false;

      // if e is not followed either by
      // '+', '-' or a digit
      if (input[i + 1] != '+' && input[i + 1] != '-' &&
          (input[i + 1] >= '0' && input[i] <= '9'))
        return false;
    }
  }

  /* If the string skips all above cases, then
  it is numeric*/
  return true;
}

Чтобы случайно сразу не прочитать ответ, добавлю картинку.

https://import.viva64.com/docx/blog/0761_Why_Code_Reviews_Are_Good_But_Not_Enough_ru/image2.png

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

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

V560 A part of conditional expression is always false: input[i] <= '9'. structopt.hpp 1870

Для тех, кто не заметил ошибку, дам пояснения. Самое главное:

else if (input[i] == 'e') {
  ....
  if (input[i + 1] != '+' && input[i + 1] != '-' &&
      (input[i + 1] >= '0' && input[i] <= '9'))
      return false;
}

Вышестоящее условие проверяет, что i-тый элемент является буквой 'e'. Соответственно следующая проверка input[i] <= '9' не имеет смысла. Результат второй проверки всегда false, о чём и предупреждает инструмент статического анализа. Причина ошибки проста: человек поспешил и опечатался, забыв написать +1.

Фактически получается, что функция не до конца выполняет свою работу по проверке корректности введённых чисел. Правильный вариант:

else if (input[i] == 'e') {
  ....
  if (input[i + 1] != '+' && input[i + 1] != '-' &&
      (input[i + 1] >= '0' && input[i + 1] <= '9'))
      return false;
}

Интересное наблюдение. Эту ошибку можно рассматривать как разновидность "эффекта последней строки". Ошибка допущена в самом последнем условии функции. В конце внимание программиста ослабло, и он допустил эту малозаметную ошибку.

GetFreeTrialImage

Если вам понравится статья про эффект последней строки, то рекомендую познакомиться с другими аналогичными наблюдениями: 0-1-2, memset, сравнения.

Всем пока. Ставлю лайк тем, кто самостоятельно нашёл ошибку.


Вы можете обсудить эту статью с другими читателями на сайте habr.com


Найдите ошибки в своем C, C++, C# и Java коде

Предлагаем попробовать проверить код вашего проекта с помощью анализатора кода PVS-Studio. Одна найденная в нём ошибка скажет вам о пользе методологии статического анализа кода больше, чем десяток статей.

goto PVS-Studio;

Андрей Карпов
Статей: 548


Найденные ошибки

Проверено проектов
411
Собрано ошибок
14 100

А ты совершаешь ошибки в коде?

Проверь с помощью
PVS-Studio

Статический анализ
кода для C, C++, C#
и Java

goto PVS-Studio;
Этот сайт использует куки и другие технологии, чтобы предоставить вам более персонализированный опыт. Продолжая просмотр страниц нашего веб-сайта, вы принимаете условия использования этих файлов. Если вы не хотите, чтобы ваши данные обрабатывались, пожалуйста, покиньте данный сайт. Подробнее →
Принять