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

Кодовые обзоры, безусловно, необходимы и полезны. Это способ поделиться знаниями, обучить, контролировать задачу, улучшить качество и форматирование кода, исправить ошибки. Более того, вы можете заметить ошибки высокого уровня, связанные с используемой архитектурой и алгоритмами. Так что это обязательная практика, за исключением того, что люди быстро устают. Поэтому статический анализ отлично дополняет обзоры и помогает обнаруживать самые разные незаметные ошибки и опечатки. Давайте посмотрим на достойный пример по этой теме.

Попробуйте найти ошибку в коде функции, взятой из библиотеки 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 Часть условного выражения всегда ложна: 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-й элемент представляет собой букву «е». Соответственно, следующая проверка 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, сравнения.

Всем пока. Престижность тем, кто сам нашел ошибку.