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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Regarding the warning discussion: It is ok to have this check here in clang-tidy first and let it mature. Once we can handle all kind of loops and do not produce false positives this logic can move into the frontend.
But in my opinion the warning-version should handle all loops. Until then we can happily have this here :)



================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:45
+
+  // We need to catch only those comparisons which contain any integer cast
+  StatementMatcher LoopVarConversionMatcher =
----------------
missing full stop.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:51
+
+  // We are interested in only those cases when the condition is a variable
+  // value (not const, enum, etc.)
----------------
same


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:60
+
+  // We use the loop increment expression only to make sure we found the right
+  // loop variable
----------------
same.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:82
+
+/// Returns the positive part of the integer width for an integer type
+unsigned calcPositiveBits(const ASTContext &Context,
----------------
same, other places too, but i won't mark them anymore.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:100
+  // Inside a binary operator ignore casting caused by constant values
+  // (constants, macros defiened values, enums, literals)
+  // We are interested in variable values' positive bits
----------------
`marco defined` is outdated.

I think the sentence should be improved. Maybe `Ignore casting cause by constant values inside a binary operator, e.g. from ... .`?


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:120
+    if (RHSEIsConstantValue && LHSEIsConstantValue)
+      return 0; // Avoid false positives produced by two constant values
+    else if (RHSEIsConstantValue)
----------------
I think that comment should be before the `if` to be consistent with other comment positions, and missing full stop.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:122
+    else if (RHSEIsConstantValue)
+      CondExprPosBits = calcPositiveBits(Context, LHSEType);
+    else if (LHSEIsConstantValue)
----------------
you can utilize early return for all these cases.
Please don't use `if`-`else` then, because no `return` after else-rule.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:142
+  if (LoopVar->getType() != LoopIncrement->getType())
+    return; // We matched the loop variable incorrectly
+
----------------
Does this try to ensure a precondition? Then it should become an assertion instead.
Please adjust the comment like above (punctuation, position)


================
Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:6
+
+Detects those ``for`` loops which has a loop variable with a "too small" type
+which means this type can't represent all values which are part of the iteration
----------------
Disclaimer: english isn't my mother tongue and its not perfect :)

`which has` -> `that have`? Sounds better to me.


================
Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:10
+
+  .. code-block:: c++
+
----------------
the `.. code-block:: c++` is usually not indended, only the code itself.


================
Comment at: test/clang-tidy/bugprone-too-small-loop-variable.cpp:5
+
+void voidBadForLoop() {
+  for (int i = 0; i < size(); ++i) {
----------------
please add tests for `range-for` loops to ensure the implicitly generated loop does not generate any false positives or the like.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53974