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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits