Pourquoi les revues de code sont bonnes, mais pas suffisantes

Les revues de code sont définitivement nécessaires et utiles. C'est l'occasion de transférer des connaissances, de la formation, de contrôler la tâche, d'améliorer la qualité et la conception du code et de corriger les erreurs. De plus, vous pouvez remarquer des erreurs de haut niveau associées à l'architecture et aux algorithmes utilisés. En général, tout va bien, mais les gens se fatiguent vite. Par conséquent, l'analyse statique est un excellent complément aux critiques et aide à révéler une variété d'erreurs et de fautes de frappe qui ne sont pas perceptibles à l'œil nu. Regardons un bon exemple sur ce sujet.



Essayez de trouver l'erreur dans le code de fonction extrait de la bibliothèque 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;
}

Afin de ne pas lire accidentellement la réponse tout de suite, je vais ajouter une image.





Je ne sais pas si vous avez trouvé une erreur ou non. Même si vous l'avez trouvé, vous conviendrez sûrement qu'il n'est pas facile de trouver une telle faute de frappe. De plus, vous saviez qu'il y avait une erreur dans la fonction. Si vous ne savez pas, alors il est difficile de vous faire lire et vérifier attentivement tout ce code.



Dans de telles situations, un analyseur de code statique complétera parfaitement la revue de code classique. L'analyseur ne se fatigue pas et vérifiera minutieusement l'intégralité du code. Par conséquent, l'analyseur PVS-Studio remarque une anomalie dans cette fonction et émet un avertissement:



V560 Une partie de l'expression conditionnelle est toujours fausse: entrée [i] <= '9'. structopt.hpp 1870



Pour ceux qui n'ont pas remarqué l'erreur, je vais donner une explication. La chose la plus importante:

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

La condition ci-dessus vérifie que le i-ème élément est la lettre «e». Par conséquent, l' entrée de contrôle suivante [i] <= '9' n'a pas de sens. Le résultat de la deuxième vérification est toujours faux, ce qui avertit l'outil d'analyse statique. La raison de l'erreur est simple: la personne s'est dépêchée et s'est scellée, oubliant d'écrire +1.



En fait, il s'avère que la fonction ne termine pas son travail de vérification de l'exactitude des nombres saisis. Option correcte:

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

Une observation intéressante. Cette erreur peut être considérée comme une variante de « l'effet de dernière ligne ». L'erreur a été commise dans la toute dernière condition de la fonction. À la fin, l'attention du programmeur a diminué et il a commis cette erreur subtile.

Si vous aimez l'article sur l'effet de la dernière ligne, je vous recommande de lire d'autres observations similaires: 0-1-2 , memset , comparaisons .



Au revoir à tout le monde. J'aime ceux qui ont trouvé l'erreur par eux-mêmes.