[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
This commit is contained in:
Balázs Kéri 2020-11-19 09:03:22 +01:00
parent 58ce4a8b11
commit 47518d6a0a
2 changed files with 59 additions and 7 deletions

View File

@ -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")));

View File

@ -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;
}