Handle do-while stmts, complex expressions and move warning to appropriate location.

Author: MalavikaSamak

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

@@ -99,7 +99,7 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
 
   auto LoopExpr =
       [](const ast_matchers::internal::Matcher<Stmt> &InnerMatcher) {
-        return stmt(anyOf(forStmt(InnerMatcher), whileStmt(InnerMatcher)));
+        return stmt(anyOf(forStmt(InnerMatcher), whileStmt(InnerMatcher), doStmt(InnerMatcher)));
       };
 
   const auto IntegerExpr = ignoringParenImpCasts(integerLiteral());
@@ -141,7 +141,9 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
 
   if (WarnOnSizeOfInLoopTermination) {
     Finder->addMatcher(
-        LoopExpr(has(binaryOperator(has(SizeOfExpr)))).bind("loop-expr"), this);
+        LoopExpr(hasDescendant(
+           binaryOperator(allOf(has(SizeOfExpr.bind("sizeof-expr")), isComparisonOperator()))
+        )).bind("loop-expr"), this);
   }
 
   // Detect sizeof(kPtr) where kPtr is 'const char* kPtr = "abc"';
@@ -369,15 +371,16 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
         << E->getSourceRange();
   } else if (const auto *E = Result.Nodes.getNodeAs<Stmt>("loop-expr")) {
     auto *SizeofArgTy = Result.Nodes.getNodeAs<Type>("sizeof-arg-type");
-    if (const auto member = dyn_cast<MemberPointerType>(SizeofArgTy)) {
+    if (const auto member = dyn_cast<MemberPointerType>(SizeofArgTy))
       SizeofArgTy = member->getPointeeType().getTypePtr();
-    }
+ 
+    auto Loc = Result.Nodes.getNodeAs<Expr>("sizeof-expr");
 
     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();
+        diag(Loc->getBeginLoc(), "suspicious usage of 'sizeof' in the loop")
+            << Loc->getSourceRange();
       }
     }
   } else if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-pointer")) {

clang-tools-extra/docs/ReleaseNotes.rst

@@ -221,9 +221,8 @@ Changes in existing checks
 
 - Improved :doc: `bugprone-sizeof-expression
  <clang-tidy/checks/bugprone/bugprone-sizeof-expression>` check.
-
- Introduced WarnOnSizeOfInLoopTermination sub-class to detect misuses of sizeof expression
- in loop conditions. 
+ Introduced WarnOnSizeOfInLoopTermination sub-class to detect misuses of sizeof
+ expression in loop conditions. 
 
 Removed checks
 ^^^^^^^^^^^^^^

clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst

@@ -320,7 +320,8 @@ Options
 .. 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.
+   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`.

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

@@ -173,44 +173,44 @@ struct B {
 };
 
 void loop_access_elements(int num, struct B b) {
-    struct A arr[10];
-    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];
-    }
+  struct A arr[10];
+  char buf[20];
+
+  // CHECK-MESSAGES: :[[@LINE+1]]:22: 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]]:22: warning: suspicious usage of 'sizeof(K)'; did you mean 'K'? [bugprone-sizeof-expression]
+  // CHECK-MESSAGES: :[[@LINE+1]]:32: 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];
+  }
 
-    // Should not warn here
-    for(int i = 0; i < sizeof(arr)/sizeof(A); 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++) {}
+  // CHECK-MESSAGES: :[[@LINE+1]]:22: 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++) {} 
+  // 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++;}
+  int i = 0;
+  // CHECK-MESSAGES: :[[@LINE+1]]:14: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression]
+  while(i <= sizeof(arr)) {i++;}
 
-    i = 0;
-    do {
-     i++;
-    } while(i <= sizeof(arr));
+  i = 0;
+  do {
+    i++;
+  // CHECK-MESSAGES: :[[@LINE+1]]:16: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression] 
+  } while(i <= sizeof(arr));
+
+  // CHECK-MESSAGES: :[[@LINE+1]]:29: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression]
+  for(int i = 0, j = 0; i < sizeof(arr) && j < sizeof(buf); i++, j++) {}
 }
 
-// Add cases for while loop
-
-// Add cases for do-while loop
-
 template <int T>
 int Foo() { int A[T]; return sizeof(T); }
 // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: suspicious usage of 'sizeof(K)'