diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp --- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp @@ -77,6 +77,10 @@ } 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 @@ 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 @@ 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 @@ 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 --- 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 Test5() { typedef int Array10[10]; + typedef C ArrayC[10]; struct MyStruct { Array10 arr; @@ -203,6 +204,8 @@ PMyStruct PS; PMyStruct2 PS2; Array10 A10; + C *PtrArray[10]; + C *PC; int sum = 0; sum += sizeof(&S.arr); @@ -239,6 +242,17 @@ // 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 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 @@ 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; }