[clang-tidy] Check for sizeof that call functions

Summary:
A common mistake that I have found in our codebase is calling a function to get an integer or enum that represents the type such as:

```
int numBytes = numElements * sizeof(x.GetType());
```

So this extends the `sizeof` check to check for these cases. There is also a `WarnOnSizeOfCall` option so it can be disabled.

Patch by Paul Fultz II!

Reviewers: hokein, alexfh, aaron.ballman, ilya-biryukov

Reviewed By: alexfh

Subscribers: lebedev.ri, xazax.hun, jkorous-apple, cfe-commits

Tags: #clang-tools-extra

Differential Revision: https://reviews.llvm.org/D44231

llvm-svn: 329073

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

@@ -62,12 +62,16 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       WarnOnSizeOfConstant(Options.get("WarnOnSizeOfConstant", 1) != 0),
+      WarnOnSizeOfIntegerExpression(
+          Options.get("WarnOnSizeOfIntegerExpression", 0) != 0),
       WarnOnSizeOfThis(Options.get("WarnOnSizeOfThis", 1) != 0),
       WarnOnSizeOfCompareToConstant(
           Options.get("WarnOnSizeOfCompareToConstant", 1) != 0) {}
 
 void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
+  Options.store(Opts, "WarnOnSizeOfIntegerExpression",
+                WarnOnSizeOfIntegerExpression);
   Options.store(Opts, "WarnOnSizeOfThis", WarnOnSizeOfThis);
   Options.store(Opts, "WarnOnSizeOfCompareToConstant",
                 WarnOnSizeOfCompareToConstant);
@@ -78,6 +82,9 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
   const auto ConstantExpr = expr(ignoringParenImpCasts(
       anyOf(integerLiteral(), unaryOperator(hasUnaryOperand(IntegerExpr)),
             binaryOperator(hasLHS(IntegerExpr), hasRHS(IntegerExpr)))));
+  const auto IntegerCallExpr = expr(ignoringParenImpCasts(
+      callExpr(anyOf(hasType(isInteger()), hasType(enumType())),
+               unless(isInTemplateInstantiation()))));
   const auto SizeOfExpr =
       expr(anyOf(sizeOfExpr(has(type())), sizeOfExpr(has(expr()))));
   const auto SizeOfZero = expr(
@@ -94,6 +101,14 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
         this);
   }
 
+  // Detect sizeof(f())
+  if (WarnOnSizeOfIntegerExpression) {
+    Finder->addMatcher(
+        expr(sizeOfExpr(ignoringParenImpCasts(has(IntegerCallExpr))))
+            .bind("sizeof-integer-call"),
+        this);
+  }
+
   // Detect expression like: sizeof(this);
   if (WarnOnSizeOfThis) {
     Finder->addMatcher(
@@ -203,6 +218,10 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-constant")) {
     diag(E->getLocStart(),
          "suspicious usage of 'sizeof(K)'; did you mean 'K'?");
+  } else if (const auto *E =
+                 Result.Nodes.getNodeAs<Expr>("sizeof-integer-call")) {
+    diag(E->getLocStart(), "suspicious usage of 'sizeof()' on an expression "
+                           "that results in an integer");
   } else if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-this")) {
     diag(E->getLocStart(),
          "suspicious usage of 'sizeof(this)'; did you mean 'sizeof(*this)'");

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

@@ -29,6 +29,7 @@ public:
 
 private:
   const bool WarnOnSizeOfConstant;
+  const bool WarnOnSizeOfIntegerExpression;
   const bool WarnOnSizeOfThis;
   const bool WarnOnSizeOfCompareToConstant;
 };

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

@@ -22,6 +22,36 @@ programmer was probably to simply get the integer and not its size.
   char buf[BUFLEN];
   memset(buf, 0, sizeof(BUFLEN));  // sizeof(42) ==> sizeof(int)
 
+Suspicious usage of 'sizeof(expr)'
+----------------------------------
+
+In cases, where there is an enum or integer to represent a type, a common
+mistake is to query the ``sizeof`` on the integer or enum that represents the
+type that should be used by ``sizeof``. This results in the size of the integer
+and not of the type the integer represents:
+
+.. code-block:: c++
+
+  enum data_type {
+    FLOAT_TYPE,
+    DOUBLE_TYPE
+  };
+
+  struct data {
+    data_type type;
+    void* buffer;
+    data_type get_type() {
+      return type;
+    }
+  };
+
+  void f(data d, int numElements) {
+    // should be sizeof(float) or sizeof(double), depending on d.get_type()
+    int numBytes = numElements * sizeof(d.get_type());
+    ...
+  }
+
+
 Suspicious usage of 'sizeof(this)'
 ----------------------------------
 
@@ -142,6 +172,11 @@ Options
    When non-zero, the check will warn on an expression like
    ``sizeof(CONSTANT)``. Default is `1`.
 
+.. option:: WarnOnSizeOfIntegerExpression
+
+   When non-zero, the check will warn on an expression like ``sizeof(expr)``
+   where the expression results in an integer. Default is `0`.
+
 .. option:: WarnOnSizeOfThis
 
    When non-zero, the check will warn on an expression like ``sizeof(this)``.

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

@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- -config="{CheckOptions: [{key: bugprone-sizeof-expression.WarnOnSizeOfIntegerExpression, value: 1}]}" --
 
 class C {
   int size() { return sizeof(this); }
@@ -14,6 +14,62 @@ extern short B[10];
 #pragma pack(1)
 struct  S { char a, b, c; };
 
+enum E { E_VALUE = 0 };
+enum class EC { VALUE = 0 };
+
+bool AsBool() { return false; }
+int AsInt() { return 0; }
+E AsEnum() { return E_VALUE; }
+EC AsEnumClass() { return EC::VALUE; }
+S AsStruct() { return {}; }
+
+struct M {
+  int AsInt() { return 0; }
+  E AsEnum() { return E_VALUE; }
+  S AsStruct() { return {}; }
+};
+
+int ReturnOverload(int) { return {}; }
+S ReturnOverload(S) { return {}; }
+
+template <class T>
+T ReturnTemplate(T) { return {}; }
+
+template <class T>
+bool TestTrait1() {
+  return sizeof(ReturnOverload(T{})) == sizeof(A);
+}
+
+template <class T>
+bool TestTrait2() {
+  return sizeof(ReturnTemplate(T{})) == sizeof(A);
+}
+
+template <class T>
+bool TestTrait3() {
+  return sizeof(ReturnOverload(0)) == sizeof(T{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+}
+
+template <class T>
+bool TestTrait4() {
+  return sizeof(ReturnTemplate(0)) == sizeof(T{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+}
+
+bool TestTemplates() {
+  bool b = true;
+  b &= TestTrait1<int>();
+  b &= TestTrait1<S>();
+  b &= TestTrait2<int>();
+  b &= TestTrait2<S>();
+  b &= TestTrait3<int>();
+  b &= TestTrait3<S>();
+  b &= TestTrait4<int>();
+  b &= TestTrait4<S>();
+  return b;
+}
+
 int Test1(const char* ptr) {
   int sum = 0;
   sum += sizeof(LEN);
@@ -22,6 +78,18 @@ int Test1(const char* ptr) {
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(K)'
   sum += sizeof(sum, LEN);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(..., ...)'
+  sum += sizeof(AsBool());
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+  sum += sizeof(AsInt());
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+  sum += sizeof(AsEnum());
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+  sum += sizeof(AsEnumClass());
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+  sum += sizeof(M{}.AsInt());
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+  sum += sizeof(M{}.AsEnum());
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
   sum += sizeof(sizeof(X));
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))'
   sum += sizeof(LEN + sizeof(X));
@@ -171,6 +239,8 @@ int ValidExpressions() {
   if (sizeof(A) < 10)
     sum += sizeof(A);
   sum += sizeof(int);
+  sum += sizeof(AsStruct());
+  sum += sizeof(M{}.AsStruct());
   sum += sizeof(A[sizeof(A) / sizeof(int)]);
   sum += sizeof(&A[sizeof(A) / sizeof(int)]);
   sum += sizeof(sizeof(0));  // Special case: sizeof size_t.