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
