[clang-tools-extra] 47518d6 - [clang-tidy] Improving bugprone-sizeof-expr check.
Balázs Kéri via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 19 01:20:28 PST 2020
Author: Balázs Kéri
Date: 2020-11-19T10:26:33+01:00
New Revision: 47518d6a0aed7ec3607aeacfa511dedda2cd67cb
URL: https://github.com/llvm/llvm-project/commit/47518d6a0aed7ec3607aeacfa511dedda2cd67cb
DIFF: https://github.com/llvm/llvm-project/commit/47518d6a0aed7ec3607aeacfa511dedda2cd67cb.diff
LOG: [clang-tidy] Improving bugprone-sizeof-expr check.
Do not warn for "pointer to aggregate" in a `sizeof(A) / sizeof(A[0])`
expression if `A` is an array of pointers. This is the usual way of
calculating the array length even if the array is of pointers.
Reviewed By: aaron.ballman
Differential Revision: https://reviews.llvm.org/D91543
Added:
Modified:
clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-sizeof-expression.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index 39526dab79ae..5790e8f84cee 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -77,6 +77,10 @@ void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
}
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
+ // FIXME:
+ // Some of the checks should not match in template code to avoid false
+ // positives if sizeof is applied on template argument.
+
const auto IntegerExpr = ignoringParenImpCasts(integerLiteral());
const auto ConstantExpr = expr(ignoringParenImpCasts(
anyOf(integerLiteral(), unaryOperator(hasUnaryOperand(IntegerExpr)),
@@ -132,6 +136,7 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
this);
// Detect sizeof(ptr) where ptr points to an aggregate (i.e. sizeof(&S)).
+ // Do not find it if RHS of a 'sizeof(arr) / sizeof(arr[0])' expression.
const auto ArrayExpr = expr(ignoringParenImpCasts(
expr(hasType(qualType(hasCanonicalType(arrayType()))))));
const auto ArrayCastExpr = expr(anyOf(
@@ -151,13 +156,31 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
hasType(qualType(hasCanonicalType(PointerToStructType))),
unless(cxxThisExpr()))));
- Finder->addMatcher(
- expr(anyOf(sizeOfExpr(has(expr(ignoringParenImpCasts(
- anyOf(ArrayCastExpr, PointerToArrayExpr, StructAddrOfExpr,
- PointerToStructExpr))))),
- sizeOfExpr(has(PointerToStructType))))
- .bind("sizeof-pointer-to-aggregate"),
- this);
+ const auto ArrayOfPointersExpr = expr(ignoringParenImpCasts(expr(hasType(
+ qualType(hasCanonicalType(arrayType(hasElementType(pointerType()))
+ .bind("type-of-array-of-pointers")))))));
+ const auto ArrayOfSamePointersExpr =
+ expr(ignoringParenImpCasts(expr(hasType(qualType(hasCanonicalType(
+ arrayType(equalsBoundNode("type-of-array-of-pointers"))))))));
+ const auto ZeroLiteral =
+ expr(ignoringParenImpCasts(integerLiteral(equals(0))));
+ const auto ArrayOfSamePointersZeroSubscriptExpr =
+ expr(ignoringParenImpCasts(arraySubscriptExpr(
+ hasBase(ArrayOfSamePointersExpr), hasIndex(ZeroLiteral))));
+ const auto ArrayLengthExprDenom =
+ expr(hasParent(expr(ignoringParenImpCasts(
+ binaryOperator(hasOperatorName("/"),
+ hasLHS(expr(ignoringParenImpCasts(expr(
+ sizeOfExpr(has(ArrayOfPointersExpr)))))))))),
+ sizeOfExpr(has(ArrayOfSamePointersZeroSubscriptExpr)));
+
+ Finder->addMatcher(expr(anyOf(sizeOfExpr(has(expr(ignoringParenImpCasts(anyOf(
+ ArrayCastExpr, PointerToArrayExpr,
+ StructAddrOfExpr, PointerToStructExpr))))),
+ sizeOfExpr(has(PointerToStructType))),
+ unless(ArrayLengthExprDenom))
+ .bind("sizeof-pointer-to-aggregate"),
+ this);
// Detect expression like: sizeof(epxr) <= k for a suspicious constant 'k'.
if (WarnOnSizeOfCompareToConstant) {
@@ -178,6 +201,12 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
this);
// Detect sizeof(...) /sizeof(...));
+ // FIXME:
+ // Re-evaluate what cases to handle by the checker.
+ // Probably any sizeof(A)/sizeof(B) should be error if
+ // 'A' is not an array (type) and 'B' the (type of the) first element of it.
+ // Except if 'A' and 'B' are non-pointers, then use the existing size division
+ // rule.
const auto ElemType =
arrayType(hasElementType(recordType().bind("elem-type")));
const auto ElemPtrType = pointerType(pointee(type().bind("elem-ptr-type")));
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 b82beccc794a..d0e60575de35 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
@@ -187,6 +187,7 @@ int Test4(const char A[10]) {
int Test5() {
typedef int Array10[10];
+ typedef C ArrayC[10];
struct MyStruct {
Array10 arr;
@@ -203,6 +204,8 @@ int Test5() {
PMyStruct PS;
PMyStruct2 PS2;
Array10 A10;
+ C *PtrArray[10];
+ C *PC;
int sum = 0;
sum += sizeof(&S.arr);
@@ -239,6 +242,17 @@ int Test5() {
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
sum += sizeof(&A10);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+ sum += sizeof(PtrArray) / sizeof(PtrArray[1]);
+ // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+ sum += sizeof(A10) / sizeof(PtrArray[0]);
+ // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+ sum += sizeof(PC) / sizeof(PtrArray[0]);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+ // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'
+ // CHECK-MESSAGES: :[[@LINE-3]]:23: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+ sum += sizeof(ArrayC) / sizeof(PtrArray[0]);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator
+ // CHECK-MESSAGES: :[[@LINE-2]]:27: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
return sum;
}
@@ -276,6 +290,10 @@ int ValidExpressions() {
int A[] = {1, 2, 3, 4};
static const char str[] = "hello";
static const char* ptr[] { "aaa", "bbb", "ccc" };
+ typedef C *CA10[10];
+ C *PtrArray[10];
+ CA10 PtrArray1;
+
int sum = 0;
if (sizeof(A) < 10)
sum += sizeof(A);
@@ -293,5 +311,10 @@ int ValidExpressions() {
sum += sizeof(str) / sizeof(str[0]);
sum += sizeof(ptr) / sizeof(ptr[0]);
sum += sizeof(ptr) / sizeof(*(ptr));
+ sum += sizeof(PtrArray) / sizeof(PtrArray[0]);
+ // Canonical type of PtrArray1 is same as PtrArray.
+ sum = sizeof(PtrArray) / sizeof(PtrArray1[0]);
+ // There is no warning for 'sizeof(T*)/sizeof(Q)' case.
+ sum += sizeof(PtrArray) / sizeof(A[0]);
return sum;
}
More information about the cfe-commits
mailing list