[PATCH] D53974: [clang-tidy] new checker: bugprone-too-small-loop-variable

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:26
+
+
+/// \brief The matcher for loops with suspicious integer loop variable.
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:41
+void TooSmallLoopVariableCheck::registerMatchers(MatchFinder *Finder) {
+
+  const StatementMatcher LoopVarMatcher =
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:85
+
+
+/// \brief Returns the positive part of the integer width for an integer type
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:88
+unsigned calcPositiveBits(const ASTContext &Context, const QualType &IntExprType) {
+
+  assert(IntExprType->isIntegerType());
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:96
+
+
+/// \brief Calculate the condition expression's positive bits, but ignore constant like values to reduce false positives
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:99
+unsigned calcCondExprPositiveBits(const ASTContext &Context, const Expr *CondExpr, const QualType &CondExprType) {
+
+  unsigned CondExprPosBits = 0;
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:105
+  // We are interested in variable values' positive bits
+  if(const auto *BinOperator = dyn_cast<BinaryOperator>(CondExpr)) {
+    const Expr *RHSE = BinOperator->getRHS()->IgnoreParenImpCasts();
----------------
Please run Clang-format.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:108
+    const Expr *LHSE = BinOperator->getLHS()->IgnoreParenImpCasts();
+
+
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:114
+
+
+    const QualType RHSEType = RHSE->getType();
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:121
+
+
+    if(RHSEType->isEnumeralType() || RHSEType.isConstQualified() ||
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:123
+    if(RHSEType->isEnumeralType() || RHSEType.isConstQualified() ||
+       RHSE->getBeginLoc().isMacroID() || isa<IntegerLiteral>(RHSE)) {
+
----------------
Braces are not needed.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:126
+      CondExprPosBits = calcPositiveBits(Context, LHSEType);
+
+    }
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:129
+    else if (LHSEType->isEnumeralType() || LHSEType.isConstQualified() ||
+             LHSE->getBeginLoc().isMacroID() || isa<IntegerLiteral>(LHSE)) {
+
----------------
Braces are not needed.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:130
+             LHSE->getBeginLoc().isMacroID() || isa<IntegerLiteral>(LHSE)) {
+
+      CondExprPosBits = calcPositiveBits(Context, RHSEType);
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:132
+      CondExprPosBits = calcPositiveBits(Context, RHSEType);
+
+    } else {
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:133
+
+    } else {
+
----------------
Braces are not needed.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:134
+    } else {
+
+      CondExprPosBits = std::max(calcPositiveBits(Context, LHSEType),
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:138
+    }
+  } else {
+
----------------
Braces are not needed.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:139
+  } else {
+
+    CondExprPosBits = calcPositiveBits(Context, CondExprType);
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:141
+    CondExprPosBits = calcPositiveBits(Context, CondExprType);
+
+  }
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:149
+void TooSmallLoopVariableCheck::check(const MatchFinder::MatchResult &Result) {
+
+  const auto *LoopVar = Result.Nodes.getNodeAs<Expr>(loopVarName);
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:153
+  const auto *LoopIncrement = Result.Nodes.getNodeAs<Expr>(loopIncrementName)->IgnoreParenImpCasts();
+
+
----------------
Please remove unnecessary empty line.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:181
+  if (LoopVarPosBits < CondExprPosBits)
+  {
+    diag(LoopVar->getBeginLoc(), "loop variable has a narrower type (%0) than "
----------------
Braces are not needed.


================
Comment at: docs/ReleaseNotes.rst:70
 
+- New :doc:`bugprone-too-small-loop-variable
+  <clang-tidy/checks/bugprone-too-small-loop-variable>` check.
----------------
Please sort list of new checks alphabetically.


================
Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:6
+
+This check searches for those for loops which has a loop variable
+with a "too small" type which means this type can't represent all values
----------------
Please synchronize with Release Notes.


================
Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:12
+    int main() {
+        long size = 294967296l;
+        for (short i = 0; i < size; ++i) {} { ... }
----------------
Please insert empty line above.


================
Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:13
+        long size = 294967296l;
+        for (short i = 0; i < size; ++i) {} { ... }
+    }
----------------
Braces are repeated twice.


================
Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:18
+values in the [0..size] interval.
+
+
----------------
Please remove unnecessary empty line.


================
Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:23
+  .. code-block:: c++
+    int doSometinhg(const std::vector& items) {
+        for (short i = 0; i < items.size(); ++i) {} { ... }
----------------
Please insert empty line above.


================
Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:24
+    int doSometinhg(const std::vector& items) {
+        for (short i = 0; i < items.size(); ++i) {} { ... }
+    }
----------------
Braces are repeated twice.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53974