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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In https://reviews.llvm.org/D53974#1285585, @ztamas wrote:

> Just to make it clear. I think this check is in a good shape now. I mean it catches usefull issues and also does not produce too many false positives, as I see.


Yes, I did not want to stop the patch or so! It would be great to remove the false-positive as they might annoy users and as consequence turn the check off.



================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:20
+
+static const char LoopName[] = "forLoopName";
+static const char loopVarName[] = "loopVar";
----------------
Please move these variable in the matcher function and make them `StringRef` instead of `const char[]`.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:40
+void TooSmallLoopVariableCheck::registerMatchers(MatchFinder *Finder) {
+  const StatementMatcher LoopVarMatcher =
+      expr(
----------------
please do not qualify values as `const`. LLVM style only qualifies pointers and references. (here and elsewhere).


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:45
+
+  // We need to catch only those comparisons when there is any integer cast
+  const StatementMatcher LoopVarConversionMatcher =
----------------
Please make all comments full sentences with punctuation and working grammar (here and elsewhere). I won't be the judge, but aaron ;)


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:82
+
+/// \brief Returns the positive part of the integer width for an integer type
+unsigned calcPositiveBits(const ASTContext &Context,
----------------
You can ellide the `\brief` here.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:106
+
+    if (RHSE->isTypeDependent() || RHSE->isValueDependent() ||
+        LHSE->isTypeDependent() || LHSE->isValueDependent())
----------------
`isInstantiationDependent()`, but please filter that already while matching.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:110
+
+    const QualType RHSEType = RHSE->getType();
+    const QualType LHSEType = LHSE->getType();
----------------
const


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:117
+    if (RHSEType->isEnumeralType() || RHSEType.isConstQualified() ||
+        RHSE->getBeginLoc().isMacroID() || isa<IntegerLiteral>(RHSE))
+      CondExprPosBits = calcPositiveBits(Context, LHSEType);
----------------
As noted on the other place, i think macros should not be ignored. If the macro defines a constant, it is handled by `IntegerLiteral`


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:134
+  const auto *CondExpr =
+      Result.Nodes.getNodeAs<Expr>(loopCondName)->IgnoreParenImpCasts();
+  const auto *LoopIncrement =
----------------
You can move the `ignoreParenImpCasts` to the matchers, there is a specific one for that.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:138
+
+  if (CondExpr->isTypeDependent() || CondExpr->isValueDependent() ||
+      LoopVar->isTypeDependent() || LoopVar->isValueDependent() ||
----------------
You can replace the two conditions with `isInstantiationDependent()`, there shuold be a matcher for that too. Ignoring them there already should be beneficial.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:149
+
+  if (CondExpr->getBeginLoc().isMacroID())
+    return; // In most of the cases a macro defines a const value, let just
----------------
I do not agree with the comment here. Macros can hide weird stuff from time to time, especially "inlined" functions. These should be analyzed as well.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:158
+
+  const unsigned LoopVarPosBits = calcPositiveBits(Context, LoopVarType);
+  const unsigned CondExprPosBits =
----------------
const


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:166
+  if (LoopVarPosBits < CondExprPosBits)
+    diag(LoopVar->getBeginLoc(), "loop variable has a narrower type (%0) than "
+                                 "the type (%1) of termination condition")
----------------
The diag can be shortened a bit `loop variable has narrower typ %0 than terminating condition %1` or similar. Diags are not sentences.


================
Comment at: docs/ReleaseNotes.rst:116
+
+  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 mark code-constructs with two backticks to emphasize them in documentation. in this case `for`


================
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
----------------
`for`


================
Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:17
+
+This for loop is an infinite loop because the short type can't represent all
+values in the [0..size] interval.
----------------
`for`, `short`


================
Comment at: test/clang-tidy/bugprone-too-small-loop-variable.cpp:6
+void voidBadForLoop() {
+  for (int i = 0; i < size(); ++i) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has a narrower type ('int') than the type ('long') of termination condition [bugprone-too-small-loop-variable]
----------------
please add tests where the rhs is a literal.


================
Comment at: test/clang-tidy/bugprone-too-small-loop-variable.cpp:152
+void voidForLoopWithEnumCond() {
+  for (short i = eSizeType::START; i < eSizeType::END; ++i) {
+  } // no warning
----------------
It is possible to specify the underlying type of an `enum`.
For the case `enum eSizeType2 : int;` the problem does occur as well. I think especially this case is tricky to spot manually and should be handled too. What do you think?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53974