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

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

Попробуйте найти ошибку в коде функции, взятой из библиотеки 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;
}

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

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

В таких ситуациях статический анализатор кода отлично дополнит классический обзор кода. Анализатор не устаёт и дотошно проверит весь код. Как результат, в этой функции анализатор 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;
}

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

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

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