[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added a comment. > Thank you very much for the patch! Thank you for reviewing! Repository: rL LLVM https://reviews.llvm.org/D53974 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth added a comment. Committed r346665. Thank you very much for the patch! Repository: rL LLVM https://reviews.llvm.org/D53974 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
This revision was automatically updated to reflect the committed changes.
Closed by commit rL346665: [clang-tidy] new check:
bugprone-too-small-loop-variable (authored by JonasToth, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D53974
Files:
clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.h
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
clang-tools-extra/trunk/test/clang-tidy/bugprone-too-small-loop-variable.cpp
Index: clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -44,6 +44,7 @@
#include "SwappedArgumentsCheck.h"
#include "TerminatingContinueCheck.h"
#include "ThrowKeywordMissingCheck.h"
+#include "TooSmallLoopVariableCheck.h"
#include "UndefinedMemoryManipulationCheck.h"
#include "UndelegatedConstructorCheck.h"
#include "UnusedRaiiCheck.h"
@@ -96,6 +97,8 @@
"bugprone-move-forwarding-reference");
CheckFactories.registerCheck(
"bugprone-multiple-statement-macro");
+CheckFactories.registerCheck(
+"bugprone-too-small-loop-variable");
CheckFactories.registerCheck(
"bugprone-narrowing-conversions");
CheckFactories.registerCheck(
Index: clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.h
+++ clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.h
@@ -0,0 +1,43 @@
+//===--- TooSmallLoopVariableCheck.h - clang-tidy ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_TOOSMALLLOOPVARIABLECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_TOOSMALLLOOPVARIABLECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// This check gives a warning if a loop variable has a too small type which
+/// might not be able to represent all values which are part of the whole range
+/// in which the loop iterates.
+/// If the loop variable's type is too small we might end up in an infinite
+/// loop. Example:
+/// \code
+/// long size = 294967296l;
+/// for (short i = 0; i < size; ++i) {} { ... }
+/// \endcode
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-too-small-loop-variable.html
+class TooSmallLoopVariableCheck : public ClangTidyCheck {
+public:
+ TooSmallLoopVariableCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_TOOSMALLLOOPVARIABLECHECK_H
Index: clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
@@ -35,6 +35,7 @@
SwappedArgumentsCheck.cpp
TerminatingContinueCheck.cpp
ThrowKeywordMissingCheck.cpp
+ TooSmallLoopVariableCheck.cpp
UndefinedMemoryManipulationCheck.cpp
UndelegatedConstructorCheck.cpp
UnusedRaiiCheck.cpp
Index: clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
@@ -0,0 +1,168 @@
+//===--- TooSmallLoopVariableCheck.cpp - clang-tidy ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "TooSmallLoopVariableCheck.h"
+#include "clang/AST/ASTContext.h"
+#include
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added a comment. > Agree. I think this check is good to go. > > I would commit this check tomorrow if that is ok with you. Of course, It would be great if this check can get in, Thanks! 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas updated this revision to Diff 173573.
ztamas added a comment.
- Add requested test case
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53974
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
clang-tidy/bugprone/TooSmallLoopVariableCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/bugprone-too-small-loop-variable.cpp
Index: test/clang-tidy/bugprone-too-small-loop-variable.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-too-small-loop-variable.cpp
@@ -0,0 +1,251 @@
+// RUN: %check_clang_tidy %s bugprone-too-small-loop-variable %t
+
+long size() { return 294967296l; }
+
+
+/// Test cases correctly caught by bugprone-too-small-loop-variable.
+
+void voidBadForLoop() {
+ for (int i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop2() {
+ for (int i = 0; i < size() + 10; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop3() {
+ for (int i = 0; i <= size() - 1; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop4() {
+ for (int i = 0; size() > i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop5() {
+ for (int i = 0; size() - 1 >= i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop6() {
+ int i = 0;
+ for (; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopUnsignedBound() {
+ unsigned size = 3147483647;
+ for (int i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'unsigned int' [bugprone-too-small-loop-variable]
+ }
+}
+
+// The iteration's upper bound has a template dependent value.
+template
+void doSomething() {
+ for (short i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+// The iteration's upper bound has a template dependent type.
+template
+void doSomething() {
+ for (T i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: loop variable has narrower type 'short' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopInstantiation() {
+ // This line does not trigger the warning.
+ doSomething();
+ // This one triggers the warning.
+ doSomething();
+}
+
+// A suspicious function used in a macro.
+#define SUSPICIOUS_SIZE (size())
+void voidBadForLoopWithMacroBound() {
+ for (short i = 0; i < SUSPICIOUS_SIZE; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+
+/// Correct loops: we should not warn here.
+
+// A simple use case when both expressions have the same type.
+void voidGoodForLoop() {
+ for (long i = 0; i < size(); ++i) { // no warning
+ }
+}
+
+// Other use case where both expressions have the same type,
+// but short expressions are converted to int by the compare operator.
+void voidGoodForLoop2() {
+ short loopCond = 10;
+ for (short i = 0; i < loopCond; ++i) { // no warning
+ }
+}
+
+// Because of the integer literal, the iteration's upper bound is int, but we suppress the warning here.
+void voidForLoopShortPlusLiteral() {
+ short size = 3;
+ for (short i = 0; i <= (size - 1); ++i) { // no warning
+ }
+}
+
+// Addition of two short variables results in an int value, but we suppress this to avoid false positives.
+void voidForLoopShortPlusShort() {
+ short size = 256;
+ short increment = 14;
+ for (short i = 0; i < size + increment; ++i) { // no warning
+ }
+}
+
+// In this test case we have different integer types, but here the loop variable has the bigger type.
+// The iteration's bound is cast implicitly, not
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth added inline comments.
Comment at: test/clang-tidy/bugprone-too-small-loop-variable.cpp:138
+ bool cond = false;
+ for (short i = 0; i < (cond ? 0 : size); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower
type 'short' than iteration's upper bound 'int'
[bugprone-too-small-loop-variable]
Could you please add a test
```
for (short i = 0; i < (!cond ? size : 0); ++i) {
}
```
as well. Just to make sure the analysis does not depent on the type of the LHS.
If it does, add a FIXME for later.
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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth added a comment. > Yes, that's right, these are not full false positives, but the check's main > focus is on those loops which are runtime dependent. If a loop's upper bound > can be calculated in compile time then this loop should be caught by a > compiler warning based on the actual value of that constant. See > -Wtautological-constant-out-of-range-compare for example. So I think it's the > best if we can avoid catching these issues using a type based matching. > Anyway, there is not too many of this kind of false positives, so it's not a > big issue. In LLVM code I did not find any similar case. > I can't see full false positives where the check works incorrectly. The > detected type mismatch seems correctly detected in every case. Agree. I think this check is good to go. I would commit this check tomorrow if that is ok with you. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added a comment. In https://reviews.llvm.org/D53974#1294659, @JonasToth wrote: > >> I would be very interested why these false positives are created. Is there > >> a way to avoid them and maybe it makes sense to already add `FIXME` at the > >> possible places the check needs improvements. > > > > voidForLoopFalsePositive() and voidForLoopFalsePositive2() test cases > > covers most of the false positives found in LibreOffice. > > I would not consider them as full false positives, the constants that are > created allow to filter more, but type-based the diagnostics are correct. So > I think that is fine. If the constants would be a big value, the check does > find a real problem. Yes, that's right, these are not full false positives, but the check's main focus is on those loops which are runtime dependent. If a loop's upper bound can be calculated in compile time then this loop should be caught by a compiler warning based on the actual value of that constant. See -Wtautological-constant-out-of-range-compare for example. So I think it's the best if we can avoid catching these issues using a type based matching. Anyway, there is not too many of this kind of false positives, so it's not a big issue. In LLVM code I did not find any similar case. I can't see full false positives where the check works incorrectly. The detected type mismatch seems correctly detected in every case. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth added a comment. >> I would be very interested why these false positives are created. Is there a >> way to avoid them and maybe it makes sense to already add `FIXME` at the >> possible places the check needs improvements. > > voidForLoopFalsePositive() and voidForLoopFalsePositive2() test cases covers > most of the false positives found in LibreOffice. I would not consider them as full false positives, the constants that are created allow to filter more, but type-based the diagnostics are correct. So I think that is fine. If the constants would be a big value, the check does find a real problem. @whisperity > I am fairly concerned the example with unsigned use for container iteration > are not false positives, just examples of bad happenstance code which never > breaks under real life applications due to uint32_t being good enough but is > actually not type-safe. > Those examples that I kept in my quote are especially bad and should be > fixed eventually... clang-tidy is not about finding _only_ bugs, but find patterns that can be problematic but are not in every instance (therefore `bugprone-` and not `bug-`). `uint32_t` does not span the whole memory-space for current hardware and a `std::string` can have more then `uint32_t::max` elements. Diagnosing this is valid. Containers where `uint32_t::max * sizeof(element_type) > size_t::max` could be filtered for normal iteration over the container. I would consider it still a bugprone pattern. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas updated this revision to Diff 173560.
ztamas added a comment.
- Fix comment
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53974
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
clang-tidy/bugprone/TooSmallLoopVariableCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/bugprone-too-small-loop-variable.cpp
Index: test/clang-tidy/bugprone-too-small-loop-variable.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-too-small-loop-variable.cpp
@@ -0,0 +1,243 @@
+// RUN: %check_clang_tidy %s bugprone-too-small-loop-variable %t
+
+long size() { return 294967296l; }
+
+
+/// Test cases correctly caught by bugprone-too-small-loop-variable.
+
+void voidBadForLoop() {
+ for (int i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop2() {
+ for (int i = 0; i < size() + 10; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop3() {
+ for (int i = 0; i <= size() - 1; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop4() {
+ for (int i = 0; size() > i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop5() {
+ for (int i = 0; size() - 1 >= i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop6() {
+ int i = 0;
+ for (; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopUnsignedBound() {
+ unsigned size = 3147483647;
+ for (int i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'unsigned int' [bugprone-too-small-loop-variable]
+ }
+}
+
+// The iteration's upper bound has a template dependent value.
+template
+void doSomething() {
+ for (short i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+// The iteration's upper bound has a template dependent type.
+template
+void doSomething() {
+ for (T i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: loop variable has narrower type 'short' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopInstantiation() {
+ // This line does not trigger the warning.
+ doSomething();
+ // This one triggers the warning.
+ doSomething();
+}
+
+// A suspicious function used in a macro.
+#define SUSPICIOUS_SIZE (size())
+void voidBadForLoopWithMacroBound() {
+ for (short i = 0; i < SUSPICIOUS_SIZE; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+
+/// Correct loops: we should not warn here.
+
+// A simple use case when both expressions have the same type.
+void voidGoodForLoop() {
+ for (long i = 0; i < size(); ++i) { // no warning
+ }
+}
+
+// Other use case where both expressions have the same type,
+// but short expressions are converted to int by the compare operator.
+void voidGoodForLoop2() {
+ short loopCond = 10;
+ for (short i = 0; i < loopCond; ++i) { // no warning
+ }
+}
+
+// Because of the integer literal, the iteration's upper bound is int, but we suppress the warning here.
+void voidForLoopShortPlusLiteral() {
+ short size = 3;
+ for (short i = 0; i <= (size - 1); ++i) { // no warning
+ }
+}
+
+// Addition of two short variables results in an int value, but we suppress this to avoid false positives.
+void voidForLoopShortPlusShort() {
+ short size = 256;
+ short increment = 14;
+ for (short i = 0; i < size + increment; ++i) { // no warning
+ }
+}
+
+// In this test case we have different integer types, but here the loop variable has the bigger type.
+// The iteration's bound is cast implicitly, not the loop
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added a comment. >> Also found some new false positives related to macros. The number of all >> false positives is around 38, which is still seems good to me. > > I would be very interested why these false positives are created. Is there a > way to avoid them and maybe it makes sense to already add `FIXME` at the > possible places the check needs improvements. voidForLoopFalsePositive() and voidForLoopFalsePositive2() test cases covers most of the false positives found in LibreOffice. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas updated this revision to Diff 173559.
ztamas added a comment.
- Add new test cases based on findings in LibreOffice code base
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53974
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
clang-tidy/bugprone/TooSmallLoopVariableCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/bugprone-too-small-loop-variable.cpp
Index: test/clang-tidy/bugprone-too-small-loop-variable.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-too-small-loop-variable.cpp
@@ -0,0 +1,243 @@
+// RUN: %check_clang_tidy %s bugprone-too-small-loop-variable %t
+
+long size() { return 294967296l; }
+
+
+/// Test cases correctly caught by bugprone-too-small-loop-variable.
+
+void voidBadForLoop() {
+ for (int i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop2() {
+ for (int i = 0; i < size() + 10; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop3() {
+ for (int i = 0; i <= size() - 1; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop4() {
+ for (int i = 0; size() > i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop5() {
+ for (int i = 0; size() - 1 >= i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop6() {
+ int i = 0;
+ for (; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopUnsignedBound() {
+ unsigned size = 3147483647;
+ for (int i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'unsigned int' [bugprone-too-small-loop-variable]
+ }
+}
+
+// The iteration's upper bound has a template dependent value.
+template
+void doSomething() {
+ for (short i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+// The iteration's upper bound has a template dependent type.
+template
+void doSomething() {
+ for (T i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: loop variable has narrower type 'short' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopInstantiation() {
+ // This line does not trigger the warning.
+ doSomething();
+ // This one triggers the warning.
+ doSomething();
+}
+
+// Suspicious funtcion defined by macro.
+#define SUSPICIOUS_SIZE (size())
+void voidBadForLoopWithMacroBound() {
+ for (short i = 0; i < SUSPICIOUS_SIZE; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+
+/// Correct loops: we should not warn here.
+
+// A simple use case when both expressions have the same type.
+void voidGoodForLoop() {
+ for (long i = 0; i < size(); ++i) { // no warning
+ }
+}
+
+// Other use case where both expressions have the same type,
+// but short expressions are converted to int by the compare operator.
+void voidGoodForLoop2() {
+ short loopCond = 10;
+ for (short i = 0; i < loopCond; ++i) { // no warning
+ }
+}
+
+// Because of the integer literal, the iteration's upper bound is int, but we suppress the warning here.
+void voidForLoopShortPlusLiteral() {
+ short size = 3;
+ for (short i = 0; i <= (size - 1); ++i) { // no warning
+ }
+}
+
+// Addition of two short variables results in an int value, but we suppress this to avoid false positives.
+void voidForLoopShortPlusShort() {
+ short size = 256;
+ short increment = 14;
+ for (short i = 0; i < size + increment; ++i) { // no warning
+ }
+}
+
+// In this test case we have different integer types, but here the loop variable has the bigger type.
+// The
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
whisperity added a comment.
In https://reviews.llvm.org/D53974#1294385, @ztamas wrote:
> I also tested on LLVm code.
> The output is here:
> https://gist.github.com/tzolnai/fe4f23031d3f9fdbdbf1ee38abda00a4
>
> I found 362 warnings.
>
> Around 95% of these warnings are similar to the next example:
>
> /home/zolnai/lohome/llvm/lib/Support/Chrono.cpp:64:24: warning: loop
> variable has narrower type 'unsigned int' than iteration's upper bound
> 'size_t' (aka 'unsigned long') [bugprone-too-small-loop-variable]
> for (unsigned I = 0; I < Style.size(); ++I) {
>
>
> Where the loop variable has an `unsigned int` type while in the loop
> condition it is compared with a container size which has `size_t` type. The
> actual size method can be `std::string::length()` or `array_lengthof()` too.
>
> //[snip snip]//
>
> I can't see similar false positives what LibreOffice code produces.
I am fairly concerned the example with unsigned use for container iteration are
not false positives, just examples of bad happenstance code which never breaks
under real life applications due to uint32_t being good enough but is actually
not type-safe.
Those examples that I kept in my quote are especially bad and should be fixed
eventually...
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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth added a comment. Thank you for checking these two projects! In https://reviews.llvm.org/D53974#1294375, @ztamas wrote: > I have the result after running the current version of the check on > LibreOffice. > > Found around 50 new issues were hidden by macros: > > https://cgit.freedesktop.org/libreoffice/core/commit/?id=afbfe42e63cdba1a18c292d7eb4875009b0f19c0 > > Also found some new false positives related to macros. The number of all > false positives is around 38, which is still seems good to me. I would be very interested why these false positives are created. Is there a way to avoid them and maybe it makes sense to already add `FIXME` at the possible places the check needs improvements. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added a comment.
I also tested on LLVm code.
The output is here:
https://gist.github.com/tzolnai/fe4f23031d3f9fdbdbf1ee38abda00a4
I found 362 warnings.
Around 95% of these warnings are similar to the next example:
/home/zolnai/lohome/llvm/lib/Support/Chrono.cpp:64:24: warning: loop variable
has narrower type 'unsigned int' than iteration's upper bound 'size_t' (aka
'unsigned long') [bugprone-too-small-loop-variable]
for (unsigned I = 0; I < Style.size(); ++I) {
Where the loop variable has an `unsigned int` type while in the loop condition
it is compared with a container size which has `size_t` type. The actualy size
method can be std::string::length() or array_lengthof() too.
An interesting catch related to a template function:
/home/zolnai/lohome/llvm/tools/clang/lib/CodeGen/CGNonTrivialStruct.cpp:310:24:
warning: loop variable has narrower type 'unsigned int' than iteration's upper
bound 'size_t' (aka 'unsigned long') [bugprone-too-small-loop-variable]
for (unsigned I = 0; I < N; ++I)
Where N is a template value with `size_t` type. If the container function is
instantiated with a "large" value I expect the function won't work. I guess it
never actually happens.
An other catch inside a macro:
/home/zolnai/lohome/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp:157:5:
warning: loop variable has narrower type 'uint32_t' (aka 'unsigned int') than
iteration's upper bound 'std::vector::size_type' (aka 'unsigned long')
[bugprone-too-small-loop-variable]
IMPLEMENT_VECTOR_INTEGER_ICMP(ne,Ty);
^
/home/zolnai/lohome/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp:123:24:
note: expanded from macro 'IMPLEMENT_VECTOR_INTEGER_ICMP'
for( uint32_t _i=0;_ihttps://reviews.llvm.org/D53974
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added a comment. I have the result after running the current version if the check. Found around 50 new issues were hidden by macros: https://cgit.freedesktop.org/libreoffice/core/commit/?id=afbfe42e63cdba1a18c292d7eb4875009b0f19c0 Also found some new false positives related to macros. The number of all false positives is around 38, which is still seems good to me. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth added a comment. > I'll run it on LibreOffice code again and we'll see. Perfect, if you have the time, running it against LLVM would be very interesting as well. >> Do you have commit access? > > Commit access? This is my first patch. If you plan to regularly contribute to LLVM you can ask for commit access (process written here: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access). I (or someone else) can commit this patch for you in the meanwhile (of course with attribution to you). 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added a comment. In https://reviews.llvm.org/D53974#1293268, @JonasToth wrote: > LGTM. > Did you run this check in its final form against a bigger project? These > results would be interesting. I'll run it on LibreOffice code again and we'll see. > Do you have commit access? Commit access? This is my first patch. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. LGTM. Did you run this check in its final form against a bigger project? These results would be interesting. Do you have commit access? 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas updated this revision to Diff 173372.
ztamas added a comment.
- Make local functions `static`
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53974
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
clang-tidy/bugprone/TooSmallLoopVariableCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/bugprone-too-small-loop-variable.cpp
Index: test/clang-tidy/bugprone-too-small-loop-variable.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-too-small-loop-variable.cpp
@@ -0,0 +1,227 @@
+// RUN: %check_clang_tidy %s bugprone-too-small-loop-variable %t
+
+long size() { return 294967296l; }
+
+
+/// Test cases correctly caught by bugprone-too-small-loop-variable.
+
+void voidBadForLoop() {
+ for (int i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop2() {
+ for (int i = 0; i < size() + 10; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop3() {
+ for (int i = 0; i <= size() - 1; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop4() {
+ for (int i = 0; size() > i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop5() {
+ for (int i = 0; size() - 1 >= i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop6() {
+ int i = 0;
+ for (; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopUnsignedBound() {
+ unsigned size = 3147483647;
+ for (int i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'unsigned int' [bugprone-too-small-loop-variable]
+ }
+}
+
+// The iteration's upper bound has a template dependent value.
+template
+void doSomething() {
+ for (short i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+// The iteration's upper bound has a template dependent type.
+template
+void doSomething() {
+ for (T i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: loop variable has narrower type 'short' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopInstantiation() {
+ // This line does not trigger the warning.
+ doSomething();
+ // This one triggers the warning.
+ doSomething();
+}
+
+
+/// Correct loops: we should not warn here.
+
+// A simple use case when both expressions have the same type.
+void voidGoodForLoop() {
+ for (long i = 0; i < size(); ++i) { // no warning
+ }
+}
+
+// Other use case where both expressions have the same type,
+// but short expressions are converted to int by the compare operator.
+void voidGoodForLoop2() {
+ short loopCond = 10;
+ for (short i = 0; i < loopCond; ++i) { // no warning
+ }
+}
+
+// Because of the integer literal, the iteration's upper bound is int, but we suppress the warning here.
+void voidForLoopShortPlusLiteral() {
+ short size = 3;
+ for (short i = 0; i <= (size - 1); ++i) { // no warning
+ }
+}
+
+// Addition of two short variables results in an int value, but we suppress this to avoid false positives.
+void voidForLoopShortPlusShort() {
+ short size = 256;
+ short increment = 14;
+ for (short i = 0; i < size + increment; ++i) { // no warning
+ }
+}
+
+// In this test case we have different integer types, but here the loop variable has the bigger type.
+// The iteration's bound is cast implicitly, not the loop variable.
+void voidForLoopBoundImplicitCast() {
+ short start = 256;
+ short end = 14;
+ for (int i = start; i >= end; --i) { // no warning
+ }
+}
+
+// Range based loop and other iterator based loops are ignored by this check.
+void voidRangeBasedForLoop() {
+ int array[] = {1, 2, 3, 4, 5};
+ for (const int :
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth added a comment. LG from my side. Please wait for feedback from @aaron.ballman or @alexfh before committing. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas updated this revision to Diff 172938.
ztamas added a comment.
- no `else` after `return`
- `static constexpr llvm::StringLiteral`
- CamelCase variable names
- Remove unneeded isIntegerType() check
- Better terminology: not terminating condition, but iteration's upper bound.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53974
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
clang-tidy/bugprone/TooSmallLoopVariableCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/bugprone-too-small-loop-variable.cpp
Index: test/clang-tidy/bugprone-too-small-loop-variable.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-too-small-loop-variable.cpp
@@ -0,0 +1,227 @@
+// RUN: %check_clang_tidy %s bugprone-too-small-loop-variable %t
+
+long size() { return 294967296l; }
+
+
+/// Test cases correctly caught by bugprone-too-small-loop-variable.
+
+void voidBadForLoop() {
+ for (int i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop2() {
+ for (int i = 0; i < size() + 10; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop3() {
+ for (int i = 0; i <= size() - 1; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop4() {
+ for (int i = 0; size() > i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop5() {
+ for (int i = 0; size() - 1 >= i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop6() {
+ int i = 0;
+ for (; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopUnsignedBound() {
+ unsigned size = 3147483647;
+ for (int i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'unsigned int' [bugprone-too-small-loop-variable]
+ }
+}
+
+// The iteration's upper bound has a template dependent value.
+template
+void doSomething() {
+ for (short i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+// The iteration's upper bound has a template dependent type.
+template
+void doSomething() {
+ for (T i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: loop variable has narrower type 'short' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopInstantiation() {
+ // This line does not trigger the warning.
+ doSomething();
+ // This one triggers the warning.
+ doSomething();
+}
+
+
+/// Correct loops: we should not warn here.
+
+// A simple use case when both expressions have the same type.
+void voidGoodForLoop() {
+ for (long i = 0; i < size(); ++i) { // no warning
+ }
+}
+
+// Other use case where both expressions have the same type,
+// but short expressions are converted to int by the compare operator.
+void voidGoodForLoop2() {
+ short loopCond = 10;
+ for (short i = 0; i < loopCond; ++i) { // no warning
+ }
+}
+
+// Because of the integer literal, the iteration's upper bound is int, but we suppress the warning here.
+void voidForLoopShortPlusLiteral() {
+ short size = 3;
+ for (short i = 0; i <= (size - 1); ++i) { // no warning
+ }
+}
+
+// Addition of two short variables results in an int value, but we suppress this to avoid false positives.
+void voidForLoopShortPlusShort() {
+ short size = 256;
+ short increment = 14;
+ for (short i = 0; i < size + increment; ++i) { // no warning
+ }
+}
+
+// In this test case we have different integer types, but here the loop variable has the bigger type.
+// The iteration's bound is cast implicitly, not the loop variable.
+void voidForLoopBoundImplicitCast() {
+ short start = 256;
+ short end = 14;
+ for (int i = start; i >= end; --i) { // no
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
lebedev.ri added inline comments.
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:20
+
+static const char LoopName[] = "forLoopName";
+static const char loopVarName[] = "loopVar";
Szelethus wrote:
> JonasToth wrote:
> > JonasToth wrote:
> > > ztamas wrote:
> > > > JonasToth wrote:
> > > > > Please move these variable in the matcher function and make them
> > > > > `StringRef` instead of `const char[]`.
> > > > These variables are used not only inside the matcher function but also
> > > > in the check() function.
> > > >
> > > I didn't see that, but they should still be `StringRef` as type.
> > One more nit i forgot: these variables should be `static` for linkage and
> > be CamelCase to match the naming conventions.
> Hmmm, `StringRef` actually has a `constexpr` constructor, we could make these
> `constexpr` too I guess.
You want `StringLiteral`:
```
static constexpr llvm::StringLiteral loopVarCastName =
llvm::StringLiteral("loopVarCast");
static constexpr llvm::StringLiteral loopCondName =
llvm::StringLiteral("loopCond");
static constexpr llvm::StringLiteral loopIncrementName =
llvm::StringLiteral("loopIncrement");
```
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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
Szelethus added inline comments. Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:20 + +static const char LoopName[] = "forLoopName"; +static const char loopVarName[] = "loopVar"; JonasToth wrote: > JonasToth wrote: > > ztamas wrote: > > > JonasToth wrote: > > > > Please move these variable in the matcher function and make them > > > > `StringRef` instead of `const char[]`. > > > These variables are used not only inside the matcher function but also in > > > the check() function. > > > > > I didn't see that, but they should still be `StringRef` as type. > One more nit i forgot: these variables should be `static` for linkage and be > CamelCase to match the naming conventions. Hmmm, `StringRef` actually has a `constexpr` constructor, we could make these `constexpr` too I guess. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth added inline comments. Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:20 + +static const char LoopName[] = "forLoopName"; +static const char loopVarName[] = "loopVar"; JonasToth wrote: > ztamas wrote: > > JonasToth wrote: > > > Please move these variable in the matcher function and make them > > > `StringRef` instead of `const char[]`. > > These variables are used not only inside the matcher function but also in > > the check() function. > > > I didn't see that, but they should still be `StringRef` as type. One more nit i forgot: these variables should be `static` for linkage and be CamelCase to match the naming conventions. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth added inline comments. Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:10 + + .. code-block:: c++ + Eugene.Zelenko wrote: > JonasToth wrote: > > ztamas wrote: > > > JonasToth wrote: > > > > the `.. code-block:: c++` is usually not indended, only the code itself. > > > Hmm, I copied this from somewhere. It might be a good idea to make this > > > consistent across all the *.rst files. See > > > bugprone-suspicious-semicolon.rst or bugprone-use-after-move.rst for > > > example. > > True, but nobody want's to do the documentation work :D > I could try to fix, but I need to be pointed to proper example :-) A nice little `sed` line i had to write 3 times fixed the thing :D 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
Eugene.Zelenko added inline comments. Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:10 + + .. code-block:: c++ + JonasToth wrote: > ztamas wrote: > > JonasToth wrote: > > > the `.. code-block:: c++` is usually not indended, only the code itself. > > Hmm, I copied this from somewhere. It might be a good idea to make this > > consistent across all the *.rst files. See > > bugprone-suspicious-semicolon.rst or bugprone-use-after-move.rst for > > example. > True, but nobody want's to do the documentation work :D I could try to fix, but I need to be pointed to proper example :-) 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth added a comment. last nits from my side (for now :)). If the other reviews could take a look at it as well, would be great. I am uncertain about the english in some comments @aaron.ballman finds all these language bugs ;) Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:123 + return calcPositiveBits(Context, RHSEType); +else + return std::max(calcPositiveBits(Context, LHSEType), please no else after return Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:126 + calcPositiveBits(Context, RHSEType)); + } else +return calcPositiveBits(Context, CondExprType); same Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:142 + if (LoopVar->getType() != LoopIncrement->getType()) +return; // We matched the loop variable incorrectly + ztamas wrote: > ztamas wrote: > > JonasToth wrote: > > > Does this try to ensure a precondition? Then it should become an > > > assertion instead. > > > Please adjust the comment like above (punctuation, position) > > It's not an assumed precondition. This `if` handles the case when > > LoopVarMatcher is not fitted with the actual loop variable. That's why the > > IncrementMatcher is there so we can check whether we found the loop > > variable. > > See voidForLoopReverseCond() test case which hits this `if` branch. > > I did not find a solution to handle this check inside the matcher. > voidForLoopReverseCond() was renamed to voidForLoopCondImplicitCast() in the > mean time. Ok, I can't think of a solution of head right now as well. It's fine to leave as is. Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:10 + + .. code-block:: c++ + ztamas wrote: > JonasToth wrote: > > the `.. code-block:: c++` is usually not indended, only the code itself. > Hmm, I copied this from somewhere. It might be a good idea to make this > consistent across all the *.rst files. See bugprone-suspicious-semicolon.rst > or bugprone-use-after-move.rst for example. True, but nobody want's to do the documentation work :D 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added inline comments. Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:142 + if (LoopVar->getType() != LoopIncrement->getType()) +return; // We matched the loop variable incorrectly + ztamas wrote: > JonasToth wrote: > > Does this try to ensure a precondition? Then it should become an assertion > > instead. > > Please adjust the comment like above (punctuation, position) > It's not an assumed precondition. This `if` handles the case when > LoopVarMatcher is not fitted with the actual loop variable. That's why the > IncrementMatcher is there so we can check whether we found the loop variable. > See voidForLoopReverseCond() test case which hits this `if` branch. > I did not find a solution to handle this check inside the matcher. voidForLoopReverseCond() was renamed to voidForLoopCondImplicitCast() in the mean time. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas updated this revision to Diff 172635.
ztamas added a comment.
- Add a range-based loop test case
- Restructure test cases a bit
- Fix-up comments, position, punctuation
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53974
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
clang-tidy/bugprone/TooSmallLoopVariableCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/bugprone-too-small-loop-variable.cpp
Index: test/clang-tidy/bugprone-too-small-loop-variable.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-too-small-loop-variable.cpp
@@ -0,0 +1,227 @@
+// RUN: %check_clang_tidy %s bugprone-too-small-loop-variable %t
+
+long size() { return 294967296l; }
+
+
+/// Test cases caught by bugprone-too-small-loop-variable.
+
+void voidBadForLoop() {
+ for (int i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop2() {
+ for (int i = 0; i < size() + 10; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop3() {
+ for (int i = 0; i <= size() - 1; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop4() {
+ for (int i = 0; size() > i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop5() {
+ for (int i = 0; size() - 1 >= i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop6() {
+ int i = 0;
+ for (; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopUnsignedCond() {
+ unsigned size = 3147483647;
+ for (int i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'unsigned int' [bugprone-too-small-loop-variable]
+ }
+}
+
+// Loop's terminating condition has a template dependent value.
+template
+void doSomething() {
+ for (short i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+// Loop's terminating condition has a template dependent type.
+template
+void doSomething() {
+ for (T i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: loop variable has narrower type 'short' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopInstantiation() {
+ // This line does not trigger the warning.
+ doSomething();
+ // This one triggers the warning.
+ doSomething();
+}
+
+
+/// Test cases where we should not warn.
+
+// A simple use case when both expressions have the same type.
+void voidGoodForLoop() {
+ for (long i = 0; i < size(); ++i) { // no warning
+ }
+}
+
+// Other use case where both expressions have the same type,
+// but short expressions are converted to int by the compare operator.
+void voidGoodForLoop2() {
+ short loopCond = 10;
+ for (short i = 0; i < loopCond; ++i) { // no warning
+ }
+}
+
+// Because of the integer literal, the loop condition is int, but we suppress the warning here.
+void voidForLoopShortPlusLiteral() {
+ short size = 3;
+ for (short i = 0; i <= (size - 1); ++i) { // no warning
+ }
+}
+
+// Addition of two short variables results in an int value, but we suppress this to avoid false positives.
+void voidForLoopShortPlusShort() {
+ short size = 256;
+ short increment = 14;
+ for (short i = 0; i < size + increment; ++i) { // no warning
+ }
+}
+
+// In this test case we have different integer types, but here the loop variable has the bigger type.
+// The terminating condition is cast implicitly, not the loop variable.
+void voidForLoopCondImplicitCast() {
+ short start = 256;
+ short end = 14;
+ for (int i = start; i >= end; --i) { // no warning
+ }
+}
+
+// Range based loop and other iterator based loops are ignored by this check.
+void voidRangeBasedForLoop() {
+ int array[] =
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added inline comments. Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:142 + if (LoopVar->getType() != LoopIncrement->getType()) +return; // We matched the loop variable incorrectly + JonasToth wrote: > Does this try to ensure a precondition? Then it should become an assertion > instead. > Please adjust the comment like above (punctuation, position) It's not an assumed precondition. This `if` handles the case when LoopVarMatcher is not fitted with the actual loop variable. That's why the IncrementMatcher is there so we can check whether we found the loop variable. See voidForLoopReverseCond() test case which hits this `if` branch. I did not find a solution to handle this check inside the matcher. Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:10 + + .. code-block:: c++ + JonasToth wrote: > the `.. code-block:: c++` is usually not indended, only the code itself. Hmm, I copied this from somewhere. It might be a good idea to make this consistent across all the *.rst files. See bugprone-suspicious-semicolon.rst or bugprone-use-after-move.rst for example. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added inline comments. Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:45 + + // We need to catch only those comparisons which contain any integer cast + StatementMatcher LoopVarConversionMatcher = JonasToth wrote: > missing full stop. Sorry, I forgot the punctuation what you've already mentioned. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
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 ,
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas marked 10 inline comments as done. ztamas added a comment. Mark all handled inline comments as Done. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
Szelethus added a comment. In https://reviews.llvm.org/D53974#1286434, @ztamas wrote: > In https://reviews.llvm.org/D53974#1285930, @Szelethus wrote: > > > In https://reviews.llvm.org/D53974#1283759, @ZaMaZaN4iK wrote: > > > > > Hmm, i thought Clang has some warning for this, but I was wrong... Did > > > you think to implement this check as Clang warning? > > > > > > That is an interesting point actually -- maybe it'd be worth doing that, > > and if more powerful analysis is required, Static Analyzer would be the > > next step. I haven't actually implemented any 'regular' clang warning, so > > I'm not sure. > > > Well, I'm implementing it as a clang-tidy check now. I guess in the future > anyone can replace it with a clang warning if he/she can implement it > effectively (e.g. no false positives). > > My first impression was that having something accepted as clang static > analyzer check takes ages and so I expect that implementing something as a > clang warning takes even more time. My impression is based on bugzilla > activity and on some read review history. It seems to me it's not rare to > have comments like: "Ping, let's not abandon this change" or the author says > that he/she has no more time for further work, etc. However clang-tidy seems > more progressive. So I prefer to have something as a clang-tidy check (and > actually get it in the upstream tool) than implementing it as a clang warning > (if it can be implemented effectively at all), wait for a year of review and > most probably abandon the change. Of course, it's just a first impression, > but why should I take the risk. I think this clang-tidy check is powerful, so > useful to have it. Okay, I'm sold on that :). 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added a comment. In https://reviews.llvm.org/D53974#1285930, @Szelethus wrote: > In https://reviews.llvm.org/D53974#1283759, @ZaMaZaN4iK wrote: > > > Hmm, i thought Clang has some warning for this, but I was wrong... Did you > > think to implement this check as Clang warning? > > > That is an interesting point actually -- maybe it'd be worth doing that, and > if more powerful analysis is required, Static Analyzer would be the next > step. I haven't actually implemented any 'regular' clang warning, so I'm not > sure. Well, I'm implementing it as a clang-tidy check now. I guess in the future anyone can replace it with a clang warning if he/she can implement it effectively (e.g. no false positives). My first impression was that having something accepted as clang static analyzer check takes ages and so I expect that implementing something as a clang warning takes even more time. My impression is based on bugzilla activity and on some read review history. It seems to me it's not rare to have comments like: "Ping, let's not abandon this change" or the author says that he/she has no more time for further work, etc. However clang-tidy seems more progressive. So I prefer to have something as a clang-tidy check (and actually get it in the upstream tool) than implementing it as a clang warning (if it can be implemented effectively at all), wait for a year of review and most probably abandon the change. Of course, it's just a first impression, but why should I take the risk. I think this clang-tidy check is powerful, so useful to have it. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas updated this revision to Diff 172487.
ztamas marked an inline comment as done.
ztamas added a comment.
Update docs based on reviewer comment
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53974
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
clang-tidy/bugprone/TooSmallLoopVariableCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/bugprone-too-small-loop-variable.cpp
Index: test/clang-tidy/bugprone-too-small-loop-variable.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-too-small-loop-variable.cpp
@@ -0,0 +1,204 @@
+// RUN: %check_clang_tidy %s bugprone-too-small-loop-variable %t
+
+long size() { return 294967296l; }
+
+void voidBadForLoop() {
+ for (int i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop2() {
+ for (int i = 0; i < size() + 10; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop3() {
+ for (int i = 0; i <= size() - 1; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop4() {
+ for (int i = 0; size() > i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop5() {
+ for (int i = 0; size() - 1 >= i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop6() {
+ int i = 0;
+ for (; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopUnsignedCond() {
+ unsigned size = 3147483647;
+ for (int i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'unsigned int' [bugprone-too-small-loop-variable]
+ }
+}
+
+// False positive: because of the integer literal, loop condition has int type
+void voidForLoopFalsePositive() {
+ short size = 3;
+ bool cond = false;
+ for (short i = 0; i < (cond ? 0 : size); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than terminating condition 'int' [bugprone-too-small-loop-variable]
+ }
+}
+
+// A loop with a template value
+template
+void doSomething() {
+ for (short i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+// A loop with a template type
+template
+void doSomething() {
+ for (T i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: loop variable has narrower type 'short' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopInstantiation() {
+ doSomething(); // does not trigger the warning
+ doSomething(); // triggers the warning
+}
+
+// Simple use case when both expressions have the same type
+void voidGoodForLoop() {
+ for (long i = 0; i < size(); ++i) {
+ } // no warning
+}
+
+// Second simple use case when both expressions have the same type
+void voidGoodForLoop2() {
+ short loopCond = 10;
+ for (short i = 0; i < loopCond; ++i) {
+ } // no warning
+}
+
+// Because of the integer literal, the loop condition is int, but we suppress the warning here
+void voidForLoopShortPlusLiteral() {
+ short size = 3;
+ for (short i = 0; i <= (size - 1); ++i) {
+ } // no warning
+}
+
+// Additions of two short variable is converted to int, but we suppress this to avoid false positives
+void voidForLoopShortPlusShort() {
+ short size = 256;
+ short increment = 14;
+ for (short i = 0; i < size + increment; ++i) {
+ } // no warning
+}
+
+// Different integer types, but in this case the loop variable is the bigger type
+void voidForLoopReverseCond() {
+ short start = 256;
+ short end = 14;
+ for (int i = start; i >= end; --i) {
+ } // no warning
+}
+
+// TODO: handle while loop
+void voidBadWhileLoop() {
+ short i = 0;
+ while (i < size()) { // missing warning
+++i;
+ }
+}
+
+// TODO: handle do-while loop
+void voidBadDoWhileLoop() {
+ short i = 0;
+ do {
+++i;
+ } while (i < size()); // missing warning
+}
+
+// Test case with a reverse
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas marked 2 inline comments as done. ztamas added inline comments. Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:109 + +if (RHSE->isInstantiationDependent() || LHSE->isInstantiationDependent()) + return 0; Szelethus wrote: > You seem to have code for handling templates, but I don't see test cases for > it. Thanks for mentioning it. I was overdefensive against false positives. This isInstantiationDependent() is not needed here. So I removed and added some test cases with templates. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas updated this revision to Diff 172486.
ztamas marked an inline comment as done.
ztamas added a comment.
- Analyze template dependant for loops too (add some test cases)
- Use double backticks for marking code in docs
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53974
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
clang-tidy/bugprone/TooSmallLoopVariableCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/bugprone-too-small-loop-variable.cpp
Index: test/clang-tidy/bugprone-too-small-loop-variable.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-too-small-loop-variable.cpp
@@ -0,0 +1,204 @@
+// RUN: %check_clang_tidy %s bugprone-too-small-loop-variable %t
+
+long size() { return 294967296l; }
+
+void voidBadForLoop() {
+ for (int i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop2() {
+ for (int i = 0; i < size() + 10; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop3() {
+ for (int i = 0; i <= size() - 1; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop4() {
+ for (int i = 0; size() > i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop5() {
+ for (int i = 0; size() - 1 >= i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop6() {
+ int i = 0;
+ for (; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopUnsignedCond() {
+ unsigned size = 3147483647;
+ for (int i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'unsigned int' [bugprone-too-small-loop-variable]
+ }
+}
+
+// False positive: because of the integer literal, loop condition has int type
+void voidForLoopFalsePositive() {
+ short size = 3;
+ bool cond = false;
+ for (short i = 0; i < (cond ? 0 : size); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than terminating condition 'int' [bugprone-too-small-loop-variable]
+ }
+}
+
+// A loop with a template value
+template
+void doSomething() {
+ for (short i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+// A loop with a template type
+template
+void doSomething() {
+ for (T i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: loop variable has narrower type 'short' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopInstantiation() {
+ doSomething(); // does not trigger the warning
+ doSomething(); // triggers the warning
+}
+
+// Simple use case when both expressions have the same type
+void voidGoodForLoop() {
+ for (long i = 0; i < size(); ++i) {
+ } // no warning
+}
+
+// Second simple use case when both expressions have the same type
+void voidGoodForLoop2() {
+ short loopCond = 10;
+ for (short i = 0; i < loopCond; ++i) {
+ } // no warning
+}
+
+// Because of the integer literal, the loop condition is int, but we suppress the warning here
+void voidForLoopShortPlusLiteral() {
+ short size = 3;
+ for (short i = 0; i <= (size - 1); ++i) {
+ } // no warning
+}
+
+// Additions of two short variable is converted to int, but we suppress this to avoid false positives
+void voidForLoopShortPlusShort() {
+ short size = 256;
+ short increment = 14;
+ for (short i = 0; i < size + increment; ++i) {
+ } // no warning
+}
+
+// Different integer types, but in this case the loop variable is the bigger type
+void voidForLoopReverseCond() {
+ short start = 256;
+ short end = 14;
+ for (int i = start; i >= end; --i) {
+ } // no warning
+}
+
+// TODO: handle while loop
+void voidBadWhileLoop() {
+ short i = 0;
+ while (i < size()) { // missing warning
+++i;
+ }
+}
+
+// TODO: handle do-while loop
+void voidBadDoWhileLoop() {
+ short i = 0;
+ do {
+++i;
+ }
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
Eugene.Zelenko added inline comments. 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 avoid This check, may be Detects? Same in documentation. 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 Somehow for is still highlighted with single `, should be ``. Same in documentation. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas marked 3 inline comments as done.
ztamas added inline comments.
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]
JonasToth wrote:
> ztamas wrote:
> > JonasToth wrote:
> > > please add tests where the rhs is a literal.
> > Do you mean tests like voidForLoopWithLiteralCond()?
> > Is it worth to add more tests like that?
> I didn't see it. In principle yes, but i would like to see a test with a
> bigger number then iterateable (maybe there is a frontend warning for that?).
> If there is no warning, this should definitely be implemented here (possible
> follow up) or even become a frontend warning.
I added a test case. This kind of test cases are caught by
-Wtautological-constant-out-of-range-compare, so we are good I 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas updated this revision to Diff 172436.
ztamas added a comment.
- Use StringRef instead of char[]
- Add test cases about big constant / literal / enum values
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53974
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
clang-tidy/bugprone/TooSmallLoopVariableCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/bugprone-too-small-loop-variable.cpp
Index: test/clang-tidy/bugprone-too-small-loop-variable.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-too-small-loop-variable.cpp
@@ -0,0 +1,184 @@
+// RUN: %check_clang_tidy %s bugprone-too-small-loop-variable %t
+
+long size() { return 294967296l; }
+
+void voidBadForLoop() {
+ for (int i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop2() {
+ for (int i = 0; i < size() + 10; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop3() {
+ for (int i = 0; i <= size() - 1; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop4() {
+ for (int i = 0; size() > i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop5() {
+ for (int i = 0; size() - 1 >= i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop6() {
+ int i = 0;
+ for (; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopUnsignedCond() {
+ unsigned size = 3147483647;
+ for (int i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'unsigned int' [bugprone-too-small-loop-variable]
+ }
+}
+
+// False positive: because of the integer literal, loop condition has int type
+void voidForLoopFalsePositive() {
+ short size = 3;
+ bool cond = false;
+ for (short i = 0; i < (cond ? 0 : size); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than terminating condition 'int' [bugprone-too-small-loop-variable]
+ }
+}
+
+// Simple use case when both expressions have the same type
+void voidGoodForLoop() {
+ for (long i = 0; i < size(); ++i) {
+ } // no warning
+}
+
+// Second simple use case when both expressions have the same type
+void voidGoodForLoop2() {
+ short loopCond = 10;
+ for (short i = 0; i < loopCond; ++i) {
+ } // no warning
+}
+
+// Because of the integer literal, the loop condition is int, but we suppress the warning here
+void voidForLoopShortPlusLiteral() {
+ short size = 3;
+ for (short i = 0; i <= (size - 1); ++i) {
+ } // no warning
+}
+
+// Additions of two short variable is converted to int, but we suppress this to avoid false positives
+void voidForLoopShortPlusShort() {
+ short size = 256;
+ short increment = 14;
+ for (short i = 0; i < size + increment; ++i) {
+ } // no warning
+}
+
+// Different integer types, but in this case the loop variable is the bigger type
+void voidForLoopReverseCond() {
+ short start = 256;
+ short end = 14;
+ for (int i = start; i >= end; --i) {
+ } // no warning
+}
+
+// TODO: handle while loop
+void voidBadWhileLoop() {
+ short i = 0;
+ while (i < size()) { // missing warning
+++i;
+ }
+}
+
+// TODO: handle do-while loop
+void voidBadDoWhileLoop() {
+ short i = 0;
+ do {
+++i;
+ } while (i < size()); // missing warning
+}
+
+// Test case with a reverse iteration
+// This is caught by -Wimplicit-int-conversion
+void voidReverseForLoop() {
+ for (short i = size() - 1; i >= 0; --i) { // no warning
+ }
+}
+
+// TODO: handle complex loop conditions
+void voidComplexForCond() {
+ bool additionalCond = true;
+ for (int i = 0; i < size() && additionalCond; ++i) { // missing warning
+ }
+}
+
+// Macro in loop condition
+#define SIZE 125
+#define SIZE2 (SIZE + 1)
+void voidForLoopWithMacroCond() {
+ for (short i = 0; i < SIZE2; ++i) { // no warning
+ }
+}
+
+// Literal in loop condition
+void voidForLoopWithLiteralCond() {
+ for (short i = 0; i < 125; ++i) { // no warning
+ }
+}
+
+// "Big" literal in
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
Szelethus added a comment. In https://reviews.llvm.org/D53974#1283759, @ZaMaZaN4iK wrote: > Hmm, i thought Clang has some warning for this, but I was wrong... Did you > think to implement this check as Clang warning? That is an interesting point actually -- maybe it'd be worth doing that, and if more powerful analysis is required, Static Analyzer would be the next step. I haven't actually implemented any 'regular' clang warning, so I'm not sure. Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:109 + +if (RHSE->isInstantiationDependent() || LHSE->isInstantiationDependent()) + return 0; You seem to have code for handling templates, but I don't see test cases for it. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth added inline comments.
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:20
+
+static const char LoopName[] = "forLoopName";
+static const char loopVarName[] = "loopVar";
ztamas wrote:
> JonasToth wrote:
> > Please move these variable in the matcher function and make them
> > `StringRef` instead of `const char[]`.
> These variables are used not only inside the matcher function but also in the
> check() function.
>
I didn't see that, but they should still be `StringRef` as type.
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]
ztamas wrote:
> JonasToth wrote:
> > please add tests where the rhs is a literal.
> Do you mean tests like voidForLoopWithLiteralCond()?
> Is it worth to add more tests like that?
I didn't see it. In principle yes, but i would like to see a test with a bigger
number then iterateable (maybe there is a frontend warning for that?). If there
is no warning, this should definitely be implemented here (possible follow up)
or even become a frontend warning.
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
ztamas wrote:
> JonasToth wrote:
> > 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?
> Hmm, it can be handled I think. However, I'm not sure how often it is, that
> an enum has an item value bigger than 32767 (short) or 65535 (unsigned short)
> or another even bigger value.
> For now, I think it's better to just ignore these cases to avoid false
> positives and keep the first version of the check simple. The scope can be
> extended anytime later I think.
`enum` values can become very big for flag-enums
```
enum Foo {
value1 = 1 << 1;
// ...
value 24 = 1 << 24;
};
```
You should still add a test for a `enum` with specified underlying type to
ensure, there is no noise.
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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas marked 16 inline comments as done.
ztamas added inline comments.
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:20
+
+static const char LoopName[] = "forLoopName";
+static const char loopVarName[] = "loopVar";
JonasToth wrote:
> Please move these variable in the matcher function and make them `StringRef`
> instead of `const char[]`.
These variables are used not only inside the matcher function but also in the
check() function.
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]
JonasToth wrote:
> please add tests where the rhs is a literal.
Do you mean tests like voidForLoopWithLiteralCond()?
Is it worth to add more tests like that?
Comment at: test/clang-tidy/bugprone-too-small-loop-variable.cpp:116
+
+// TODO: handle those cases when the loop condition contains a floating point
expression
+void voidDoubleForCond() {
whisperity wrote:
> (Misc: You might want to check out D52730 for floats later down the line.)
In the meantime, I thought this float comparison is not a real use case. It
just came in my mind while I was trying to find out test cases. So I just
removed it. The referenced conversion check might catch it anyway.
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
JonasToth wrote:
> 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?
Hmm, it can be handled I think. However, I'm not sure how often it is, that an
enum has an item value bigger than 32767 (short) or 65535 (unsigned short) or
another even bigger value.
For now, I think it's better to just ignore these cases to avoid false
positives and keep the first version of the check simple. The scope can be
extended anytime later I 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas updated this revision to Diff 172422.
ztamas added a comment.
Update code based on reviewer comments:
- Remove const qualifier
- Fix some comments
- Use isInstantiationDependent() method
- Do not ignore macros
- Mark code-constructs in docs
- Handle the use case when both operands of the binary operator is constant
inside the condition expression.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53974
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
clang-tidy/bugprone/TooSmallLoopVariableCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/bugprone-too-small-loop-variable.cpp
Index: test/clang-tidy/bugprone-too-small-loop-variable.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-too-small-loop-variable.cpp
@@ -0,0 +1,154 @@
+// RUN: %check_clang_tidy %s bugprone-too-small-loop-variable %t
+
+long size() { return 294967296l; }
+
+void voidBadForLoop() {
+ for (int i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop2() {
+ for (int i = 0; i < size() + 10; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop3() {
+ for (int i = 0; i <= size() - 1; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop4() {
+ for (int i = 0; size() > i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop5() {
+ for (int i = 0; size() - 1 >= i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop6() {
+ int i = 0;
+ for (; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopUnsignedCond() {
+ unsigned size = 3147483647;
+ for (int i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'unsigned int' [bugprone-too-small-loop-variable]
+ }
+}
+
+// False positive: because of the integer literal, loop condition has int type
+void voidForLoopFalsePositive() {
+ short size = 3;
+ bool cond = false;
+ for (short i = 0; i < (cond ? 0 : size); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than terminating condition 'int' [bugprone-too-small-loop-variable]
+ }
+}
+
+// Simple use case when both expressions have the same type
+void voidGoodForLoop() {
+ for (long i = 0; i < size(); ++i) {
+ } // no warning
+}
+
+// Second simple use case when both expressions have the same type
+void voidGoodForLoop2() {
+ short loopCond = 10;
+ for (short i = 0; i < loopCond; ++i) {
+ } // no warning
+}
+
+// Because of the integer literal, the loop condition is int, but we suppress the warning here
+void voidForLoopShortPlusLiteral() {
+ short size = 3;
+ for (short i = 0; i <= (size - 1); ++i) {
+ } // no warning
+}
+
+// Additions of two short variable is converted to int, but we suppress this to avoid false positives
+void voidForLoopShortPlusShort() {
+ short size = 256;
+ short increment = 14;
+ for (short i = 0; i < size + increment; ++i) {
+ } // no warning
+}
+
+// Different integer types, but in this case the loop variable is the bigger type
+void voidForLoopReverseCond() {
+ short start = 256;
+ short end = 14;
+ for (int i = start; i >= end; --i) {
+ } // no warning
+}
+
+// TODO: handle while loop
+void voidBadWhileLoop() {
+ short i = 0;
+ while (i < size()) { // missing warning
+++i;
+ }
+}
+
+// TODO: handle do-while loop
+void voidBadDoWhileLoop() {
+ short i = 0;
+ do {
+++i;
+ } while (i < size()); // missing warning
+}
+
+// We do not catch this, but other conversion related checks can catch it anyway
+void voidReverseForLoop() {
+ for (short i = size() - 1; i >= 0; --i) { // no warning
+ }
+}
+
+// TODO: handle complex loop conditions
+void voidComplexForCond() {
+ bool additionalCond = true;
+ for (int i = 0; i < size() && additionalCond; ++i) { // missing warning
+ }
+}
+
+// Macro in loop condition
+#define SIZE 125
+#define SIZE2 (SIZE + 1)
+void voidForLoopWithMacroCond() {
+ for (short i = 0; i <
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
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 ,
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(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(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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
whisperity added inline comments.
Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:18
+This for loop is an infinite loop because the short type can't represent all
+values in the [0..size] interval.
+
Format the interval as code (monospace): `[0 .. size]`
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) {}
There is a typo in the function's name.
Comment at: test/clang-tidy/bugprone-too-small-loop-variable.cpp:116
+
+// TODO: handle those cases when the loop condition contains a floating point
expression
+void voidDoubleForCond() {
(Misc: You might want to check out D52730 for floats later down the line.)
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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added a comment. 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. So my intention here is to get it in as it is now (of course after I fixed things reviewers find) and later it can be improved in separate patches (e.g. reduce false positives even more, handle other use cases like while loop etc.). I think it would be better to have smaller patches than polishing this big one until it gets ~prefect. In the test file I added some TODO comments. They were added as a documentation of future possibilities. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas marked 30 inline comments as done. ztamas added a comment. I fixed up formatting and the docs based on comments. 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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas updated this revision to Diff 172226.
ztamas added a comment.
Run clang-formats on files, remove unneeded empty lines, fix up documentation.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53974
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
clang-tidy/bugprone/TooSmallLoopVariableCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/bugprone-too-small-loop-variable.cpp
Index: test/clang-tidy/bugprone-too-small-loop-variable.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-too-small-loop-variable.cpp
@@ -0,0 +1,161 @@
+// RUN: %check_clang_tidy %s bugprone-too-small-loop-variable %t
+
+long size() { return 294967296l; }
+
+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]
+ }
+}
+
+void voidBadForLoop2() {
+ for (int i = 0; i < size() + 10; ++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]
+ }
+}
+
+void voidBadForLoop3() {
+ for (int i = 0; i <= size() - 1; ++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]
+ }
+}
+
+void voidBadForLoop4() {
+ for (int i = 0; size() > i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: loop variable has a narrower type ('int') than the type ('long') of termination condition [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop5() {
+ for (int i = 0; size() - 1 >= i; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: loop variable has a narrower type ('int') than the type ('long') of termination condition [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidBadForLoop6() {
+ int i = 0;
+ for (; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: loop variable has a narrower type ('int') than the type ('long') of termination condition [bugprone-too-small-loop-variable]
+ }
+}
+
+void voidForLoopUnsignedCond() {
+ unsigned size = 3147483647;
+ for (int i = 0; i < size; ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has a narrower type ('int') than the type ('unsigned int') of termination condition [bugprone-too-small-loop-variable]
+ }
+}
+
+// False positive: because of the integer literal, loop condition has int type
+void voidForLoopFalsePositive() {
+ short size = 3;
+ bool cond = false;
+ for (short i = 0; i < (cond ? 0 : size); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has a narrower type ('short') than the type ('int') of termination condition [bugprone-too-small-loop-variable]
+ }
+}
+
+// Simple use case when both expressions have the same type
+void voidGoodForLoop() {
+ for (long i = 0; i < size(); ++i) {
+ } // no warning
+}
+
+// Second simple use case when both expressions have the same type
+void voidGoodForLoop2() {
+ short loopCond = 10;
+ for (short i = 0; i < loopCond; ++i) {
+ } // no warning
+}
+
+// Because of the integer literal, the loop condition is int, but we suppress the warning here
+void voidForLoopShortPlusLiteral() {
+ short size = 3;
+ for (short i = 0; i <= (size - 1); ++i) {
+ } // no warning
+}
+
+// Additions of two short variable is converted to int, but we suppress this to avoid false positives
+void voidForLoopShortPlusShort() {
+ short size = 256;
+ short increment = 14;
+ for (short i = 0; i < size + increment; ++i) {
+ } // no warning
+}
+
+// Different integer types, but in this case the loop variable is the bigger type
+void voidForLoopReverseCond() {
+ short start = 256;
+ short end = 14;
+ for (int i = start; i >= end; --i) {
+ } // no warning
+}
+
+// TODO: handle while loop
+void voidBadWhileLoop() {
+ short i = 0;
+ while (i < size()) {
+++i;
+ } // missing warning
+}
+
+// TODO: handle do-while loop
+void voidBadDoWhileLoop() {
+ short i = 0;
+ do {
+++i;
+ } while (i < size()); // missing warning
+}
+
+// We do not catch this, but other conversion related checks can catch it anyway
+void voidReverseForLoop() {
+ for (short i = size() - 1; i >= 0; --i) {
+ } // no warning
+}
+
+// TODO: handle those cases when the loop condition contains a floating point expression
+void voidDoubleForCond() {
+ double condVar = size();
+ for (short i = 0; i < condVar; ++i) {
+ } // missing warning
+}
+
+// TODO: handle complex loop conditions
+void voidComplexForCond() {
+ bool additionalCond = true;
+ for (int i = 0; i < size() && additionalCond; ++i) {
+ } //
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth added a comment.
But that reasoning would apply to `if` and `while` loops, as it might be
an "always true". But you are right, this case is not generally problematic.
long size = 30;
short index = 100;
// Opposite comparison
if(index > size) {
//
}
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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added a comment.
In https://reviews.llvm.org/D53974#1284000, @JonasToth wrote:
> For general understanding: Couldn't this check be generalized to comparing
> integers of different sizes? We tried a 'dont-mix-int-types' check for
> arithmetic already, its complicated :)
> But this as a specialization of the category could be done easier (i think).
>
> What do you think?
I don't think so. This comparison is suspicious only inside a loop, not in
general.
For example see this code:
long size = 30;
short index = 100;
if(index < size) {
//
}
You can choose the two values as you want, this comparison will work correctly.
However in a loop condition this comparison means a problem, because the loop
stops only if the "index" variable gets bigger than the "size" variable.
So the loop context is important here.
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
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth added a comment. For general understanding: Couldn't this check be generalized to comparing integers of different sizes? We tried a 'dont-mix-int-types' check for arithmetic already, its complicated :) But this as a specialization of the category could be done easier (i think). 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
