Warum Code-Reviews gut sind, aber nicht genug

Codeüberprüfungen sind definitiv erforderlich und nützlich. Dies ist eine Gelegenheit, Wissen zu übertragen, zu schulen, die Aufgabe zu kontrollieren, die Qualität und das Design des Codes zu verbessern und Fehler zu beheben. Darüber hinaus können Sie Fehler auf hoher Ebene feststellen, die mit der verwendeten Architektur und den verwendeten Algorithmen verbunden sind. Im Allgemeinen ist alles in Ordnung, aber die Leute werden schnell müde. Daher ist die statische Analyse eine hervorragende Ergänzung zu Überprüfungen und hilft, eine Vielzahl von Fehlern und Tippfehlern zu identifizieren, die für das Auge nicht erkennbar sind. Schauen wir uns ein gutes Beispiel zu diesem Thema an.



Versuchen Sie, den Fehler im Funktionscode aus der structopt- Bibliothek zu finden :

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

Um die Antwort nicht versehentlich sofort zu lesen, füge ich ein Bild hinzu.





Ich weiß nicht, ob Sie einen Fehler gefunden haben oder nicht. Selbst wenn Sie es gefunden haben, werden Sie sicherlich zustimmen, dass es nicht einfach ist, einen solchen Tippfehler zu finden. Außerdem wussten Sie, dass ein Fehler in der Funktion vorliegt. Wenn Sie es nicht wissen, ist es schwierig, den gesamten Code sorgfältig zu lesen und zu überprüfen.



In solchen Situationen ergänzt ein statischer Code-Analysator die klassische Code-Überprüfung perfekt. Der Analysator wird nicht müde und überprüft den gesamten Code gründlich. Infolgedessen bemerkt der PVS-Studio-Analysator eine Anomalie in dieser Funktion und gibt eine Warnung aus:



V560 Ein Teil des bedingten Ausdrucks ist immer falsch: Eingabe [i] <= '9'. structopt.hpp 1870



Für diejenigen, die den Fehler nicht bemerkt haben, werde ich eine Erklärung geben. Das Wichtigste:

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

Die obige Bedingung prüft, ob das i-te Element der Buchstabe 'e' ist. Dementsprechend ist die folgende Prüfeingabe [i] <= '9' nicht sinnvoll. Das Ergebnis der zweiten Prüfung ist immer falsch, worüber das statische Analysetool warnt. Der Grund für den Fehler ist einfach: Die Person beeilte sich und versiegelte sich und vergaß, +1 zu schreiben.



Tatsächlich stellt sich heraus, dass die Funktion die Überprüfung der Richtigkeit der eingegebenen Zahlen nicht abgeschlossen hat. Richtige Option:

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

Eine interessante Beobachtung. Dieser Fehler kann als Variation des " Last-Line-Effekts " angesehen werden. Der Fehler wurde im allerletzten Zustand der Funktion gemacht. Am Ende ließ die Aufmerksamkeit des Programmierers nach und er machte diesen subtilen Fehler.

Wenn Ihnen der Artikel über die Wirkung der letzten Zeile gefällt , empfehle ich, andere ähnliche Beobachtungen zu lesen : 0-1-2 , Memset , Vergleiche .



Auf Wiedersehen an alle. Ich mag diejenigen, die den Fehler selbst gefunden haben.