Index: clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.h +++ clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.h @@ -29,6 +29,7 @@ private: const bool WarnOnSizeOfConstant; + const bool WarnOnSizeOfIntegerExpression; const bool WarnOnSizeOfThis; const bool WarnOnSizeOfCompareToConstant; }; Index: clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.cpp @@ -62,12 +62,16 @@ 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 @@ 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 @@ 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 @@ if (const auto *E = Result.Nodes.getNodeAs("sizeof-constant")) { diag(E->getLocStart(), "suspicious usage of 'sizeof(K)'; did you mean 'K'?"); + } else if (const auto *E = + Result.Nodes.getNodeAs("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("sizeof-this")) { diag(E->getLocStart(), "suspicious usage of 'sizeof(this)'; did you mean 'sizeof(*this)'"); Index: clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-sizeof-expression.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-sizeof-expression.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-sizeof-expression.rst @@ -22,6 +22,36 @@ 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 @@ 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)``. Index: clang-tools-extra/trunk/test/clang-tidy/bugprone-sizeof-expression.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/bugprone-sizeof-expression.cpp +++ clang-tools-extra/trunk/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 @@ #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 +T ReturnTemplate(T) { return {}; } + +template +bool TestTrait1() { + return sizeof(ReturnOverload(T{})) == sizeof(A); +} + +template +bool TestTrait2() { + return sizeof(ReturnTemplate(T{})) == sizeof(A); +} + +template +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 +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(); + b &= TestTrait1(); + b &= TestTrait2(); + b &= TestTrait2(); + b &= TestTrait3(); + b &= TestTrait3(); + b &= TestTrait4(); + b &= TestTrait4(); + return b; +} + int Test1(const char* ptr) { int sum = 0; sum += sizeof(LEN); @@ -22,6 +78,18 @@ // 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 @@ 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.