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

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

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

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

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


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

    Всем пока. Ставлю лайк тем, кто самостоятельно нашёл ошибку.
    PVS-Studio
    Статический анализ кода для C, C++, C# и Java

    Комментарии 11

      +1

      Похоже, в условии пропущено отрицание:
      if (input[i + 1] != '+' && input[i + 1] != '-' &&
      (input[i + 1] >= '0' && input[i] <= '9')) // <--
      return false;


      !(input[i + 1] >= '0' && input[i+1] <= '9')

        +8
        обзоры кода

        Только с третьего раза понял о чём речь. И вроде перевод полностью корректный, но осознал, что так вообще никто вокруг меня не говорит

          +4
          Вот тут if конечно забавен — он никогда не сработает. Это типичная ошибка при использовании unsigned типов. Проверка призвана ловить строки из одних пробелов, но j улетает в небеса (-1).

          PVS-Studio ловит такое?

            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;
          
            +3
            Такое пока нет. ;) Но, спасибо.
              +1

              Еще забавный порядок действий:


                if (is_binary_notation(input) ||
                    is_hex_notation(input) ||
                    is_octal_notation(input)) {
                  return true;
                }
              
                if (input.empty()) {
                  return false;
                }

              Сначала выполнили кучу действий и только потом проверили пустая строка или нет.

              +6

              Честно говоря, до ревью такой код не должен доходить вообще, что это за "ненормальное программирование"? У ребят вообще стандарт кодирования был? Сколько тут цикломатическая сложность? Даже смотреть бы код не стали бы, сразу на переработку… и искать тогда ничего не надо было бы в этой лапше.

                +2

                Это, безусловно, главное. Но даже если смотреть, утирая кровавые слёзы, чтоб, как попросили "найти ошибку", то их там же полно:


                • цикл удаления пробелов с конца проскакивает начало строки
                • dot_or_exp не ставится в true на '.'
                • проверка на "last character" должна быть i + 1 >= input.length()
                • и вообще-то должна быть проверка на предпоследний символ, чтоб можно было смотреть input[i+1]
                • то же самое ниже, в проверке на 'e'
                • 1e+ проверку пройдёт, 1e3e4,1e6figvam8, 1foo2.4 — тоже

                Главное, конечно, не ошибки, а сам подход в топку.

                +3

                Мне казалось "ревью кода" стало общепринятым термином. Полез в английскую версию и только так понял, что "обзор кода"="code review"

                  +3

                  Можно ли считать ошибкой что любая каша из цифр и знаков плюс минус считается "валидным числом"?
                  Про строку из одних пробелов писали выше — по логике там вообще вылет будет.

                    0
                    Возможно там никогда не попадает такое значение в функцию

                    UPD даже скорее всего не приходит. Это часть парсера, так что пробелы должны быть уже отсечены где-то выше.
                    0
                    Итак, в комментариях к этой статье люди писали, что там есть и другие места, которые по всей видимости являются ошибками. И что вообще код написан сложно и неаккуратно. Очень рад, что данная тема вызвала интерес. Поэтому в продолжение предлагаю публикацию «Почему важно проводить статический анализ открытых библиотек, которые вы добавляете в свой проект» :). habr.com/ru/company/pvs-studio/blog/520498

                    Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                    Самое читаемое