[clang-tidy] Extend bugprone-sizeof-expression to check sizeof(pointers to structures)

[baloghadamsoftware]

Summary

Accidentally taking the size of a struct-pointer type or a value of this type is more common than explicitly using the & operator for the value. This patch extends the check to include these cases.

[whisperity]

test/clang-tidy/bugprone-sizeof-expression.cpp

196 ↗

(On Diff #197101)

While I trust Clang and the matchers to unroll the type and still match, I'd prefer also adding a test case for

typedef TMyStruct* PMyStruct2;

or somesuch.

And perhaps a "copy" of these cases where they come from template arguments, in case the checker can also warn for that?

231 ↗

(On Diff #197101)

Why is this printed at sizeof(A*)? Do we not print the name of the actual type used in the expression?

[aaron.ballman]

Aside from the testing request, this LGTM.

test/clang-tidy/bugprone-sizeof-expression.cpp

231 ↗

(On Diff #197101)

The original check did this, so it's not really new behavior here. I don't recall the rationale off the top of my head, but I'd guess it's because the type is not really salient, but the fact that the type is a pointer is.

[baloghadamsoftware]

New tests added.

[baloghadamsoftware]

test/clang-tidy/bugprone-sizeof-expression.cpp

196 ↗

(On Diff #197101)

I added the new test, but templates are outside the scope of this particular patch. They should be tested (and fixed if the tests fail) in another patch.

[aaron.ballman]

LGTM!

clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.cpp

const auto ArrayCastExpr = expr(anyOf(
       castExpr(hasSourceExpression(ArrayExpr))));
   const auto PointerToArrayExpr = expr(ignoringParenImpCasts(expr(
       hasType(qualType(hasCanonicalType(pointerType(pointee(arrayType()))))))));
 
   const auto StructAddrOfExpr =
       unaryOperator(hasOperatorName("&"),
                     hasUnaryOperand(ignoringParenImpCasts(expr(
                         hasType(qualType(hasCanonicalType(recordType())))))));
+  const auto PointerToStructType = type(hasUnqualifiedDesugaredType(
+      pointerType(pointee(recordType()))));
+  const auto PointerToStructExpr = expr(ignoringParenImpCasts(expr(
+      hasType(qualType(hasCanonicalType(PointerToStructType))),
+      unless(cxxThisExpr()))));
 
   Finder->addMatcher(
-      expr(sizeOfExpr(has(expr(ignoringParenImpCasts(
-               anyOf(ArrayCastExpr, PointerToArrayExpr, StructAddrOfExpr))))))
+      expr(anyOf(sizeOfExpr(has(expr(ignoringParenImpCasts(
+               anyOf(ArrayCastExpr, PointerToArrayExpr, StructAddrOfExpr,
+                     PointerToStructExpr))))),
+                            sizeOfExpr(has(PointerToStructType))))
           .bind("sizeof-pointer-to-aggregate"),
       this);
 
   // Detect expression like: sizeof(epxr) <= k for a suspicious constant 'k'.
   if (WarnOnSizeOfCompareToConstant) {
     Finder->addMatcher(
         binaryOperator(matchers::isRelationalOperator(),
                        hasEitherOperand(ignoringParenImpCasts(SizeOfExpr)),

clang-tools-extra/trunk/test/clang-tidy/bugprone-sizeof-expression.cpp

 int Test5() {
   typedef int Array10[10];
 
   struct MyStruct {
     Array10 arr;
     Array10* ptr;
   };
   typedef const MyStruct TMyStruct;
+  typedef const MyStruct *PMyStruct;
+  typedef TMyStruct *PMyStruct2;
 
   static TMyStruct kGlocalMyStruct = {};
   static TMyStruct volatile * kGlocalMyStructPtr = &kGlocalMyStruct;
 
   MyStruct S;
+  PMyStruct PS;
+  PMyStruct2 PS2;
   Array10 A10;
 
   int sum = 0;
   sum += sizeof(&S.arr);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(&kGlocalMyStruct.arr);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(&kGlocalMyStructPtr->arr);
int Test5() {
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(kGlocalMyStructPtr->ptr);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
 
   sum += sizeof(&kGlocalMyStruct);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(&S);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(MyStruct*);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PMyStruct);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PS2);
+  // 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
 
   return sum;
 }
 
 int Test6() {
   int sum = 0;