https://github.com/malavikasamak updated
https://github.com/llvm/llvm-project/pull/143205
>From 52e4413ea1e701dfe0b24cf957a26bb72732f066 Mon Sep 17 00:00:00 2001
From: MalavikaSamak <malavi...@apple.com>
Date: Wed, 21 May 2025 16:06:44 -0700
Subject: [PATCH 1/3] Place holder message for sizeof operator in loops.
---
.../bugprone/SizeofExpressionCheck.cpp | 17 +++++++++-
.../bugprone/SizeofExpressionCheck.h | 1 +
.../checkers/bugprone/sizeof-expression.cpp | 31 +++++++++++++++++++
3 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index f3d4c2255d86e..ee66a880792b8 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -72,7 +72,8 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
Options.get("WarnOnSizeOfPointerToAggregate", true)),
WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)),
WarnOnOffsetDividedBySizeOf(
- Options.get("WarnOnOffsetDividedBySizeOf", true)) {}
+ Options.get("WarnOnOffsetDividedBySizeOf", true)),
+ WarnOnLoopExprSizeOf(Options.get("WarnOnLoopExprSizeOf", true)) {}
void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -86,6 +87,7 @@ void
SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
Options.store(Opts, "WarnOnOffsetDividedBySizeOf",
WarnOnOffsetDividedBySizeOf);
+ Options.store(Opts, "WarnOnLoopExprSizeOf", WarnOnLoopExprSizeOf);
}
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -93,6 +95,11 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder
*Finder) {
// Some of the checks should not match in template code to avoid false
// positives if sizeof is applied on template argument.
+ auto LoopExpr =
+ [](const ast_matchers::internal::Matcher<Stmt> &InnerMatcher) {
+ return stmt(anyOf(forStmt(InnerMatcher), whileStmt(InnerMatcher)));
+ };
+
const auto IntegerExpr = ignoringParenImpCasts(integerLiteral());
const auto ConstantExpr = ignoringParenImpCasts(
anyOf(integerLiteral(), unaryOperator(hasUnaryOperand(IntegerExpr)),
@@ -130,6 +137,11 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder
*Finder) {
this);
}
+ if (WarnOnLoopExprSizeOf) {
+ Finder->addMatcher(
+ LoopExpr(has(binaryOperator(has(SizeOfExpr)))).bind("loop-expr"),
this);
+ }
+
// Detect sizeof(kPtr) where kPtr is 'const char* kPtr = "abc"';
const auto CharPtrType = pointerType(pointee(isAnyCharacter()));
const auto ConstStrLiteralDecl =
@@ -353,6 +365,9 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult &Result) {
diag(E->getBeginLoc(),
"suspicious usage of 'sizeof(char*)'; do you mean 'strlen'?")
<< E->getSourceRange();
+ } else if (const auto *E = Result.Nodes.getNodeAs<Stmt>("loop-expr")) {
+ diag(E->getBeginLoc(), "suspicious usage of 'sizeof' in the loop")
+ << E->getSourceRange();
} else if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-pointer")) {
if (Result.Nodes.getNodeAs<Type>("struct-type")) {
diag(E->getBeginLoc(),
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
index fbd62cb80fb2d..f7dccf39687a5 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
@@ -32,6 +32,7 @@ class SizeofExpressionCheck : public ClangTidyCheck {
const bool WarnOnSizeOfPointerToAggregate;
const bool WarnOnSizeOfPointer;
const bool WarnOnOffsetDividedBySizeOf;
+ const bool WarnOnLoopExprSizeOf;
};
} // namespace clang::tidy::bugprone
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
index 5e6f394152e9d..52b71277466b1 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -164,6 +164,37 @@ int Test2(MyConstChar* A) {
return sum;
}
+struct A {
+ int array[10];
+};
+
+struct B {
+ struct A a;
+};
+
+int square(int num, struct B b) {
+ struct A arr[10];
+ // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: suspicious usage of 'sizeof'
in the loop [bugprone-sizeof-expression]
+ for(int i = 0; i < sizeof(arr); i++) {
+ struct A a = arr[i];
+ }
+ // CHECK-MESSAGES: :[[@LINE+2]]:24: warning: suspicious usage of
'sizeof(K)'; did you mean 'K'? [bugprone-sizeof-expression]
+ // CHECK-MESSAGES: :[[@LINE+1]]:34: warning: suspicious usage of
'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator
[bugprone-sizeof-expression]
+ for(int i = 0; i < sizeof(10)/sizeof(A); i++) {
+ struct A a = arr[i];
+ }
+
+ for(int i = 0; i < sizeof(arr)/sizeof(A); i++) {
+ struct A a = arr[i];
+ }
+
+ // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: suspicious usage of 'sizeof'
in the loop [bugprone-sizeof-expression]
+ for(int j = 0; j < sizeof(b.a); j++) {
+
+ }
+ return 2;
+}
+
template <int T>
int Foo() { int A[T]; return sizeof(T); }
// CHECK-MESSAGES: :[[@LINE-1]]:30: warning: suspicious usage of 'sizeof(K)'
>From b7677bbd0f6f687ac4c96614d22b3af74e11a601 Mon Sep 17 00:00:00 2001
From: MalavikaSamak <malavi...@apple.com>
Date: Fri, 6 Jun 2025 14:07:58 -0700
Subject: [PATCH 2/3] Add documentation for the newly introduced CheckOption
for loop.
---
.../clang-tidy/bugprone/SizeofExpressionCheck.cpp | 8 ++++----
.../clang-tidy/bugprone/SizeofExpressionCheck.h | 2 +-
.../docs/clang-tidy/checks/bugprone/sizeof-expression.rst | 8 ++++++++
.../clang-tidy/checkers/bugprone/sizeof-expression.cpp | 4 ++--
4 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index ee66a880792b8..6a0ecf6b1c460 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -73,7 +73,7 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)),
WarnOnOffsetDividedBySizeOf(
Options.get("WarnOnOffsetDividedBySizeOf", true)),
- WarnOnLoopExprSizeOf(Options.get("WarnOnLoopExprSizeOf", true)) {}
+
WarnOnSizeOfInLoopTermination(Options.get("WarnOnSizeOfInLoopTermination",
true)) {}
void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -87,7 +87,7 @@ void
SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
Options.store(Opts, "WarnOnOffsetDividedBySizeOf",
WarnOnOffsetDividedBySizeOf);
- Options.store(Opts, "WarnOnLoopExprSizeOf", WarnOnLoopExprSizeOf);
+ Options.store(Opts, "WarnOnSizeOfInLoopTermination",
WarnOnSizeOfInLoopTermination);
}
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -137,9 +137,9 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder
*Finder) {
this);
}
- if (WarnOnLoopExprSizeOf) {
+ if (WarnOnSizeOfInLoopTermination) {
Finder->addMatcher(
- LoopExpr(has(binaryOperator(has(SizeOfExpr)))).bind("loop-expr"),
this);
+ LoopExpr(has(binaryOperator(has(SizeOfExpr.bind("loop-expr"))))),
this);
}
// Detect sizeof(kPtr) where kPtr is 'const char* kPtr = "abc"';
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
index f7dccf39687a5..e979b4723cf2e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
@@ -32,7 +32,7 @@ class SizeofExpressionCheck : public ClangTidyCheck {
const bool WarnOnSizeOfPointerToAggregate;
const bool WarnOnSizeOfPointer;
const bool WarnOnOffsetDividedBySizeOf;
- const bool WarnOnLoopExprSizeOf;
+ const bool WarnOnSizeOfInLoopTermination;
};
} // namespace clang::tidy::bugprone
diff --git
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst
index 29edb26ed7aa2..68340f04489fb 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst
@@ -316,3 +316,11 @@ Options
When `true`, the check will warn on pointer arithmetic where the
element count is obtained from a division with ``sizeof(...)``,
e.g., ``Ptr + Bytes / sizeof(*T)``. Default is `true`.
+
+.. option:: WarnOnSizeOfInLoopTermination
+
+ When `true`, the check will warn about incorrect use of sizeof expression
+ in loop termination condition. The warning triggers if the sizeof expression
+ appears to be incorrectly used to determine the number of array/buffer
elements.
+ e.g, ``long arr[10]; for(int i = 0; i < sizeof(arr); i++) { ... }. Default
+ is `true`.
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
index 52b71277466b1..82d1fba11872c 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -174,7 +174,7 @@ struct B {
int square(int num, struct B b) {
struct A arr[10];
- // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: suspicious usage of 'sizeof'
in the loop [bugprone-sizeof-expression]
+ // CHECK-MESSAGES: :[[@LINE+1]]:24: warning: suspicious usage of 'sizeof'
in the loop [bugprone-sizeof-expression]
for(int i = 0; i < sizeof(arr); i++) {
struct A a = arr[i];
}
@@ -188,7 +188,7 @@ int square(int num, struct B b) {
struct A a = arr[i];
}
- // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: suspicious usage of 'sizeof'
in the loop [bugprone-sizeof-expression]
+ // CHECK-MESSAGES: :[[@LINE+1]]:24: warning: suspicious usage of 'sizeof'
in the loop [bugprone-sizeof-expression]
for(int j = 0; j < sizeof(b.a); j++) {
}
>From df65e16fb70a62e5c6d75d8b8dfca28450f78614 Mon Sep 17 00:00:00 2001
From: MalavikaSamak <malavi...@apple.com>
Date: Mon, 9 Jun 2025 16:41:43 -0700
Subject: [PATCH 3/3] Added more tests and handled false positives.
---
.../bugprone/SizeofExpressionCheck.cpp | 22 +++++++++---
.../checkers/bugprone/sizeof-expression.cpp | 36 +++++++++++++------
2 files changed, 43 insertions(+), 15 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index 6a0ecf6b1c460..4d2d16b83e475 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -73,7 +73,8 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)),
WarnOnOffsetDividedBySizeOf(
Options.get("WarnOnOffsetDividedBySizeOf", true)),
-
WarnOnSizeOfInLoopTermination(Options.get("WarnOnSizeOfInLoopTermination",
true)) {}
+ WarnOnSizeOfInLoopTermination(
+ Options.get("WarnOnSizeOfInLoopTermination", true)) {}
void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -87,7 +88,8 @@ void
SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
Options.store(Opts, "WarnOnOffsetDividedBySizeOf",
WarnOnOffsetDividedBySizeOf);
- Options.store(Opts, "WarnOnSizeOfInLoopTermination",
WarnOnSizeOfInLoopTermination);
+ Options.store(Opts, "WarnOnSizeOfInLoopTermination",
+ WarnOnSizeOfInLoopTermination);
}
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -139,7 +141,7 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder
*Finder) {
if (WarnOnSizeOfInLoopTermination) {
Finder->addMatcher(
- LoopExpr(has(binaryOperator(has(SizeOfExpr.bind("loop-expr"))))),
this);
+ LoopExpr(has(binaryOperator(has(SizeOfExpr)))).bind("loop-expr"),
this);
}
// Detect sizeof(kPtr) where kPtr is 'const char* kPtr = "abc"';
@@ -366,8 +368,18 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult &Result) {
"suspicious usage of 'sizeof(char*)'; do you mean 'strlen'?")
<< E->getSourceRange();
} else if (const auto *E = Result.Nodes.getNodeAs<Stmt>("loop-expr")) {
- diag(E->getBeginLoc(), "suspicious usage of 'sizeof' in the loop")
- << E->getSourceRange();
+ auto *SizeofArgTy = Result.Nodes.getNodeAs<Type>("sizeof-arg-type");
+ if (const auto member = dyn_cast<MemberPointerType>(SizeofArgTy)) {
+ SizeofArgTy = member->getPointeeType().getTypePtr();
+ }
+
+ if (const auto type = dyn_cast<ArrayType>(SizeofArgTy)) {
+ CharUnits sSize = Ctx.getTypeSizeInChars(type->getElementType());
+ if (!sSize.isOne()) {
+ diag(E->getBeginLoc(), "suspicious usage of 'sizeof' in the loop")
+ << E->getSourceRange();
+ }
+ }
} else if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-pointer")) {
if (Result.Nodes.getNodeAs<Type>("struct-type")) {
diag(E->getBeginLoc(),
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
index 82d1fba11872c..5ae11d6759373 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -172,28 +172,44 @@ struct B {
struct A a;
};
-int square(int num, struct B b) {
+void loop_access_elements(int num, struct B b) {
struct A arr[10];
- // CHECK-MESSAGES: :[[@LINE+1]]:24: warning: suspicious usage of 'sizeof'
in the loop [bugprone-sizeof-expression]
+ char buf[20];
+
+ // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: suspicious usage of 'sizeof'
in the loop [bugprone-sizeof-expression]
for(int i = 0; i < sizeof(arr); i++) {
struct A a = arr[i];
}
+
+ // Loop warning should not trigger here, even though this code is incorrect
// CHECK-MESSAGES: :[[@LINE+2]]:24: warning: suspicious usage of
'sizeof(K)'; did you mean 'K'? [bugprone-sizeof-expression]
// CHECK-MESSAGES: :[[@LINE+1]]:34: warning: suspicious usage of
'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator
[bugprone-sizeof-expression]
for(int i = 0; i < sizeof(10)/sizeof(A); i++) {
struct A a = arr[i];
}
- for(int i = 0; i < sizeof(arr)/sizeof(A); i++) {
- struct A a = arr[i];
- }
+ // Should not warn here
+ for(int i = 0; i < sizeof(arr)/sizeof(A); i++) {}
+
+ // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: suspicious usage of 'sizeof'
in the loop [bugprone-sizeof-expression]
+ for(int j = 0; j < sizeof(b.a.array); j++) {}
+
+ // Should not warn here
+ for(int i = 0; i < sizeof(buf); i++) {}
+
+ int i = 0;
+ // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: suspicious usage of 'sizeof'
in the loop [bugprone-sizeof-expression]
+ while(i <= sizeof(arr)) {i++;}
+
+ i = 0;
+ do {
+ i++;
+ } while(i <= sizeof(arr));
+}
- // CHECK-MESSAGES: :[[@LINE+1]]:24: warning: suspicious usage of 'sizeof'
in the loop [bugprone-sizeof-expression]
- for(int j = 0; j < sizeof(b.a); j++) {
+// Add cases for while loop
- }
- return 2;
-}
+// Add cases for do-while loop
template <int T>
int Foo() { int A[T]; return sizeof(T); }
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
