Por qué las revisiones de código son buenas, pero no suficientes

Las revisiones de código son definitivamente necesarias y útiles. Esta es una oportunidad para transferir conocimientos, capacitación, control sobre la tarea, mejorar la calidad y el diseño del código y corregir errores. Además, puede notar errores de alto nivel asociados con la arquitectura y los algoritmos utilizados. En general, todo está bien, pero la gente se cansa rápidamente. Por lo tanto, el análisis estático es un excelente complemento para las revisiones y ayuda a revelar una variedad de errores y errores tipográficos que no se notan a simple vista. Veamos un buen ejemplo sobre este tema.



Intente encontrar el error en el código de función tomado de la biblioteca de 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;
}

Para no leer accidentalmente la respuesta de inmediato, agregaré una imagen.





No sé si encontró un error o no. Incluso si lo ha encontrado, seguramente estará de acuerdo en que no es fácil encontrar un error tipográfico de este tipo. Además, sabía que había un error en la función. Si no lo sabe, entonces es difícil conseguir que lea y verifique cuidadosamente todo este código.



En tales situaciones, un analizador de código estático complementará perfectamente la revisión de código clásica. El analizador no se cansa y comprobará minuciosamente todo el código. Como resultado, el analizador PVS-Studio detecta una anomalía en esta función y emite una advertencia:



V560 Una parte de la expresión condicional es siempre falsa: input [i] <= '9'. structopt.hpp 1870



Para aquellos que no notaron el error, daré una explicación. La cosa más importante:

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

La condición anterior verifica que el elemento i-ésimo sea la letra 'e'. En consecuencia, la siguiente entrada de verificación [i] <= '9' no tiene sentido. El resultado de la segunda comprobación es siempre falso, que es lo que advierte la herramienta de análisis estático. La razón del error es simple: la persona se apresuró y se selló, olvidándose de escribir +1.



De hecho, resulta que la función no completa su trabajo de verificar la exactitud de los números ingresados. Opción correcta:

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

Una observación interesante. Este error puede verse como una variación del " efecto de última línea ". El error se cometió en la última condición de la función. Al final, la atención del programador se desvaneció y cometió este sutil error.

Si te gusta el artículo sobre el efecto de la última línea, te recomiendo leer otras observaciones similares: 0-1-2 , memset , comparaciones .



Adiós a todos. Me gustan aquellos que encontraron el error por sí mismos.