Clever Geek Handbook

コードレビューは良いが十分ではない理由

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条件式の一部は常に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」であることを確認します。したがって、次のチェック入力[i] <= '9'は意味がありません。2番目のチェックの結果は常に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、comparisons。



みなさん、さようなら。私は自分でエラーを見つけた人が好きです。


More articles:


All Articles