Author: hokein Date: Tue Apr 3 08:10:24 2018 New Revision: 329073 URL: http://llvm.org/viewvc/llvm-project?rev=329073&view=rev Log: [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 Modified: clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.cpp clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.h clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-sizeof-expression.rst clang-tools-extra/trunk/test/clang-tidy/bugprone-sizeof-expression.cpp Modified: clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.cpp?rev=329073&r1=329072&r2=329073&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.cpp Tue Apr 3 08:10:24 2018 @@ -62,12 +62,16 @@ SizeofExpressionCheck::SizeofExpressionC 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::registerMatc 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::registerMatc 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 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)'"); Modified: clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.h?rev=329073&r1=329072&r2=329073&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.h (original) +++ clang-tools-extra/trunk/clang-tidy/bugprone/SizeofExpressionCheck.h Tue Apr 3 08:10:24 2018 @@ -29,6 +29,7 @@ public: private: const bool WarnOnSizeOfConstant; + const bool WarnOnSizeOfIntegerExpression; const bool WarnOnSizeOfThis; const bool WarnOnSizeOfCompareToConstant; }; Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-sizeof-expression.rst URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-sizeof-expression.rst?rev=329073&r1=329072&r2=329073&view=diff ============================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-sizeof-expression.rst (original) +++ clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-sizeof-expression.rst Tue Apr 3 08:10:24 2018 @@ -22,6 +22,36 @@ programmer was probably to simply get th 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)``. Modified: clang-tools-extra/trunk/test/clang-tidy/bugprone-sizeof-expression.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/bugprone-sizeof-expression.cpp?rev=329073&r1=329072&r2=329073&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/bugprone-sizeof-expression.cpp (original) +++ clang-tools-extra/trunk/test/clang-tidy/bugprone-sizeof-expression.cpp Tue Apr 3 08:10:24 2018 @@ -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. _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
