Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -31,6 +31,7 @@ StringConstructorCheck.cpp StringIntegerAssignmentCheck.cpp StringLiteralWithEmbeddedNulCheck.cpp + SuspiciousEnumUsageCheck.cpp SuspiciousMissingCommaCheck.cpp SuspiciousSemicolonCheck.cpp SuspiciousStringCompareCheck.cpp Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -39,6 +39,7 @@ #include "StringConstructorCheck.h" #include "StringIntegerAssignmentCheck.h" #include "StringLiteralWithEmbeddedNulCheck.h" +#include "SuspiciousEnumUsageCheck.h" #include "SuspiciousMissingCommaCheck.h" #include "SuspiciousSemicolonCheck.h" #include "SuspiciousStringCompareCheck.h" @@ -115,6 +116,8 @@ "misc-string-integer-assignment"); CheckFactories.registerCheck( "misc-string-literal-with-embedded-nul"); + CheckFactories.registerCheck( + "misc-suspicious-enum-usage"); CheckFactories.registerCheck( "misc-suspicious-missing-comma"); CheckFactories.registerCheck( Index: clang-tidy/misc/SuspiciousEnumUsageCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/SuspiciousEnumUsageCheck.h @@ -0,0 +1,42 @@ +//===--- SuspiciousEnumUsageCheck.h - clang-tidy--------------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SUSPICIOUS_ENUM_USAGE_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SUSPICIOUS_ENUM_USAGE_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// The checker detects various cases when an enum is probably misused (as a +/// bitmask). +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-suspicious-enum-usage.html +class SuspiciousEnumUsageCheck : public ClangTidyCheck { +public: + SuspiciousEnumUsageCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + +private: + const bool StrictMode; + const char* DifferentEnumErrorMessage; + const char* BitmaskErrorMessage; + const char* BitmaskVarErrorMessage; + const char* BitmaskNoteMessage; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SUSPICIOUS_ENUM_USAGE_H Index: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/SuspiciousEnumUsageCheck.cpp @@ -0,0 +1,239 @@ +//===--- SuspiciousEnumUsageCheck.cpp - clang-tidy-------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "SuspiciousEnumUsageCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +/// Stores a min and a max value which describe an interval. +struct ValueRange { + llvm::APSInt MinVal; + llvm::APSInt MaxVal; + + ValueRange(const EnumDecl *EnumDec) { + llvm::APSInt BeginVal = EnumDec->enumerator_begin()->getInitVal(); + + const auto MinMaxVal = std::minmax_element( + EnumDec->enumerator_begin(), EnumDec->enumerator_end(), + [](const EnumConstantDecl *E1, const EnumConstantDecl *E2) { + return E1->getInitVal() < E2->getInitVal(); + }); + MinVal = MinMaxVal.first->getInitVal(); + MaxVal = MinMaxVal.second->getInitVal(); + } +}; + +/// Return the number of EnumConstantDecls in an EnumDecl. +static int enumLength(const EnumDecl *EnumDec) { + return std::distance(EnumDec->enumerator_begin(), EnumDec->enumerator_end()); +} + +static bool hasDisjointValueRange(const EnumDecl *Enum1, + const EnumDecl *Enum2) { + ValueRange Range1(Enum1), Range2(Enum2); + return (Range1.MaxVal < Range2.MinVal) || (Range2.MaxVal < Range1.MinVal); +} + +static bool hasCommonBit(const llvm::APSInt &Val1, const llvm::APSInt &Val2) { + return (Val1 & Val2).getExtValue(); +} +static bool isNonPowerOf2NorNullLiteral(const EnumConstantDecl *EnumConst) { + if (const Expr *InitExpr = EnumConst->getInitExpr()) { + llvm::APSInt Val = EnumConst->getInitVal(); + return Val.getBoolValue() && !Val.isPowerOf2() && + dyn_cast_or_null(InitExpr->IgnoreImpCasts()); + } + return false; +} +static bool isMaxValAllBitSetLiteral(const EnumDecl *EnumDec) { + auto EnumConst = std::max_element( + EnumDec->enumerator_begin(), EnumDec->enumerator_end(), + [](const EnumConstantDecl *E1, const EnumConstantDecl *E2) { + return E1->getInitVal() < E2->getInitVal(); + }); + if (const Expr *InitExpr = EnumConst->getInitExpr()) + return EnumConst->getInitVal().countTrailingOnes() == + EnumConst->getInitVal().getActiveBits() && + dyn_cast(InitExpr->IgnoreImpCasts()); + return false; +} + +static int countNonPowOfTwoLiteralNum(const EnumDecl *EnumDec) { + return std::count_if( + EnumDec->enumerator_begin(), EnumDec->enumerator_end(), + [](const EnumConstantDecl *E) { return isNonPowerOf2NorNullLiteral(E); }); +} + +/// Check if there is one or two enumerators that are not a power of 2 and are +/// initialized by a literal in the enum type, and that the enumeration contains +/// enough elements to reasonably act as a bitmask. Exclude the case where the +/// last enumerator is the sum of the lesser values (and initialized by a +/// literal) or when it could contain consecutive values. +static bool isPossiblyBitMask(int NonPowOfTwoCounter, int EnumLen, + const ValueRange &VR, const EnumDecl *EnumDec) { + return NonPowOfTwoCounter >= 1 && NonPowOfTwoCounter <= 2 && + NonPowOfTwoCounter < enumLength(EnumDec) / 2 && + (VR.MaxVal - VR.MinVal != EnumLen - 1) && + !(NonPowOfTwoCounter == 1 && isMaxValAllBitSetLiteral(EnumDec)); +} + +SuspiciousEnumUsageCheck::SuspiciousEnumUsageCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), StrictMode(Options.get("StrictMode", 1)) { + DifferentEnumErrorMessage = "enum values are from different enum types"; + + BitmaskErrorMessage = "enum type seems like a bitmask (contains mostly " + "power-of-2 literals), but this literal is not a " + "power-of-2"; + BitmaskNoteMessage = "used here as a bitmask"; + + BitmaskVarErrorMessage = + "enum type seems like a bitmask (contains mostly " + "power-of-2 literals) but some literal(s) are not a power-of-2"; +} + +void SuspiciousEnumUsageCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "StrictMode", StrictMode); +} + +void SuspiciousEnumUsageCheck::registerMatchers(MatchFinder *Finder) { + const auto enumExpr = [](StringRef RefName, StringRef DeclName) { + return allOf(ignoringImpCasts(expr().bind(RefName)), + ignoringImpCasts(hasType(enumDecl().bind(DeclName)))); + }; + + Finder->addMatcher( + binaryOperator(hasOperatorName("|"), hasLHS(enumExpr("", "enumDecl")), + hasRHS(allOf(enumExpr("", "otherEnumDecl"), + ignoringImpCasts(hasType(enumDecl( + unless(equalsBoundNode("enumDecl")))))))) + .bind("diffEnumOp"), + this); + + Finder->addMatcher( + binaryOperator(anyOf(hasOperatorName("+"), hasOperatorName("|")), + hasLHS(enumExpr("lhsExpr", "enumDecl")), + hasRHS(allOf(enumExpr("rhsExpr", ""), + ignoringImpCasts(hasType( + enumDecl(equalsBoundNode("enumDecl"))))))) + .bind("enumBinOp"), + this); + + Finder->addMatcher( + binaryOperator(anyOf(hasOperatorName("+"), hasOperatorName("|")), + hasEitherOperand( + allOf(hasType(isInteger()), unless(enumExpr("", "")))), + hasEitherOperand(enumExpr("enumExpr", "enumDecl"))), + this); + + Finder->addMatcher( + binaryOperator(anyOf(hasOperatorName("|="), hasOperatorName("+=")), + hasRHS(enumExpr("enumExpr", "enumDecl"))), + this); +} + +void SuspiciousEnumUsageCheck::check(const MatchFinder::MatchResult &Result) { + // Case 1: The two enum values come from different types. + if (const auto *DiffEnumOp = + Result.Nodes.getNodeAs("diffEnumOp")) { + const auto *EnumDec = Result.Nodes.getNodeAs("enumDecl"); + const auto *OtherEnumDec = + Result.Nodes.getNodeAs("otherEnumDecl"); + + if (EnumDec->enumerator_begin() == EnumDec->enumerator_end() || + OtherEnumDec->enumerator_begin() == OtherEnumDec->enumerator_end()) + return; + + if (!hasDisjointValueRange(EnumDec, OtherEnumDec)) + diag(DiffEnumOp->getOperatorLoc(), + "enum values are from different enum types"); + return; + } + + // Case 2: + // a. Investigating the right hand side of `+=` or `|=` operator. + // b. When the operator is `|` or `+` but only one of them is an EnumExpr + if (const auto *EnumExpr = Result.Nodes.getNodeAs("enumExpr")) { + if (!StrictMode) + return; + const auto *EnumDec = Result.Nodes.getNodeAs("enumDecl"); + ValueRange VR(EnumDec); + int EnumLen = enumLength(EnumDec); + int NonPowOfTwoCounter = countNonPowOfTwoLiteralNum(EnumDec); + if (isPossiblyBitMask(NonPowOfTwoCounter, EnumLen, VR, EnumDec)) { + const auto *EnumDecExpr = dyn_cast(EnumExpr); + const auto *EnumConstDecl = + EnumDecExpr ? dyn_cast(EnumDecExpr->getDecl()) + : nullptr; + + if (EnumConstDecl && isNonPowerOf2NorNullLiteral(EnumConstDecl)) { + diag(EnumConstDecl->getSourceRange().getBegin(), BitmaskErrorMessage); + diag(EnumExpr->getExprLoc(), BitmaskNoteMessage, DiagnosticIDs::Note); + } else if (!EnumConstDecl) { + diag(EnumDec->getInnerLocStart(), BitmaskVarErrorMessage); + diag(EnumExpr->getExprLoc(), BitmaskNoteMessage, DiagnosticIDs::Note); + } + } + return; + } + + // Case 3: + // '|' or '+' operator where both argument comes from the same enum type + const auto *EnumBinOp = Result.Nodes.getNodeAs("enumBinOp"); + + const auto *EnumDec = Result.Nodes.getNodeAs("enumDecl"); + + const auto *LhsExpr = Result.Nodes.getNodeAs("lhsExpr"); + const auto *RhsExpr = Result.Nodes.getNodeAs("rhsExpr"); + + const auto *LhsDecExpr = dyn_cast(LhsExpr); + const auto *RhsDecExpr = dyn_cast(RhsExpr); + + const auto *LhsConstant = + LhsDecExpr ? dyn_cast(LhsDecExpr->getDecl()) : nullptr; + const auto *RhsConstant = + RhsDecExpr ? dyn_cast(RhsDecExpr->getDecl()) : nullptr; + bool LhsVar = !LhsConstant, RhsVar = !RhsConstant; + + if (!StrictMode && (LhsVar || RhsVar)) + return; + + int NonPowOfTwoCounter = countNonPowOfTwoLiteralNum(EnumDec); + ValueRange VR(EnumDec); + int EnumLen = enumLength(EnumDec); + if (isPossiblyBitMask(NonPowOfTwoCounter, EnumLen, VR, EnumDec) && + (StrictMode || + (EnumBinOp->isBitwiseOp() && RhsConstant && LhsConstant && + hasCommonBit(LhsConstant->getInitVal(), RhsConstant->getInitVal())))) { + if (LhsVar) { + diag(EnumDec->getInnerLocStart(), BitmaskVarErrorMessage); + diag(LhsExpr->getExprLoc(), BitmaskNoteMessage, DiagnosticIDs::Note); + } else if (isNonPowerOf2NorNullLiteral(LhsConstant)) { + diag(LhsConstant->getSourceRange().getBegin(), BitmaskErrorMessage); + diag(LhsExpr->getExprLoc(), BitmaskNoteMessage, DiagnosticIDs::Note); + } + if (RhsVar) { + diag(EnumDec->getInnerLocStart(), BitmaskVarErrorMessage); + diag(RhsExpr->getExprLoc(), BitmaskNoteMessage, DiagnosticIDs::Note); + } else if (isNonPowerOf2NorNullLiteral(RhsConstant)) { + diag(RhsConstant->getSourceRange().getBegin(), BitmaskErrorMessage); + diag(LhsExpr->getExprLoc(), BitmaskNoteMessage, DiagnosticIDs::Note); + } + } +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -80,6 +80,7 @@ misc-string-constructor misc-string-integer-assignment misc-string-literal-with-embedded-nul + misc-suspicious-enum-usage misc-suspicious-missing-comma misc-suspicious-semicolon misc-suspicious-string-compare Index: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-suspicious-enum-usage.rst @@ -0,0 +1,71 @@ +.. title:: clang-tidy - misc-suspicious-enum-usage + +misc-suspicious-enum-usage +================ + +The checker detects various cases when an enum is probably misused (as a bitmask). + +1. When "ADD" or "bitwise OR" is used between two enum which come from different + types and these types value ranges are not disjoint. + +In the following cases you can choose either "Strict" or "Weak" option. +In "Strict" mode we check if the used EnumConstantDecl type contains almost +only pow-of-2 numbers. +We regard the enum as a bitmask if the two conditions below are true at the same time: + +* at most one third of the elements of the enum are non pow-of-2 numbers (because of short enumerations) +* there is only one non pow-of-2 number except for the enum constant representing all choices (the result "bitwise OR" operation of all enum elements) + +So whenever the non pow-of-2 element is used we diagnose a misuse and give a warning. + +In the "Weak" case we only say it is misused if on the both side of the `|` +operator the EnumConstantDecls have common bit so we could lose information +(and all the "Strict" conditions). + +2. Investigating the right hand side of `+=` or `|=` operator. (only in "Strict"). +3. Check only the enum value side of a `|` or `+` operator if one of them is not + enum val. (only in "Strict") +4. Check both side of `|` or `+` operator where the enum values are from the same + enum type. + +=============== + +Examples: + +.. code-block:: c++ + + enum { A, B, C }; + enum { D, E, F = 5 }; + enum { G = 10, H = 11, I = 12 }; + + unsigned flag; + flag = + A | + H; // OK, disjoint value intervalls in the enum types -> probably good use. + flag = B | F; // Warning, have common values so they are probably misused. + + // Case 2 (only in strict mode): + enum bitmask { + A = 0, + B = 1, + C = 2, + D = 4, + E = 8, + F = 16, + G = 31 // OK, real bitmask. + }; + + enum Almostbitmask { + AA = 0, + BB = 1, + CC = 2, + DD = 4, + EE = 8, + FF = 16, + GG // Problem, forgot to initialize. + }; + + unsigned flag = 0; + flag |= E; // OK. + flag |= + EE; // Warning at the decl, and note that it was used here as a bitmask. Index: test/clang-tidy/misc-suspicious-enum-usage-strict.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-suspicious-enum-usage-strict.cpp @@ -0,0 +1,92 @@ +// RUN: %check_clang_tidy %s misc-suspicious-enum-usage %t -- -config="{CheckOptions: [{key: misc-suspicious-enum-usage.StrictMode, value: 1}]}" -- + +enum A { + A = 1, + B = 2, + C = 4, + D = 8, + E = 16, + F = 32, + G = 63 +}; + +enum X { + X = 8, + Y = 16, + Z = 4, + ZZ = 3 + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask (contains mostly power-of-2 literals), but this literal is not a power-of-2 [misc-suspicious-enum-usage] +}; +// CHECK-MESSAGES: :[[@LINE+1]]:1: warning: enum type seems like a bitmask (contains mostly power-of-2 literals) but some literal(s) are not a power-of-2 +enum PP { + P = 2, + Q = 3, + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask (contains mostly power-of-2 literals), but this literal is not a power-of-2 + R = 4, + S = 8, + T = 16, + U = 31 +}; + +enum { + H, + I, + J, + K, + L +}; + +enum Days { + Monday, + Tuesday, + Wednesday, + Thursday, + Friday, + Saturday, + Sunday +}; + +Days bestDay() { + return Friday; +} + +int trigger() { + if (bestDay() | A) + return 1; + // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types + if (I | Y) + return 1; + // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types + if (P + Q == R) + return 1; + else if ((Q | R) == T) + return 1; + else + int k = ZZ | Z; + unsigned p = R; + PP pp = Q; + p |= pp; + // [LINE-1] triggers the LINE:17 warning + p = A | G; + return 0; +} + +int dont_trigger() { + int a = 1, b = 5; + int c = a + b; + int d = c | H, e = b * a; + a = B | C; + b = X | Z; + + unsigned bitflag; + enum A aa = B; + bitflag = aa | C; + + if (Tuesday != Monday + 1 || + Friday - Thursday != 1 || + Sunday + Wednesday == (Sunday | Wednesday)) + return 1; + if (H + I + L == 42) + return 1; + return 42; +} Index: test/clang-tidy/misc-suspicious-enum-usage.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-suspicious-enum-usage.cpp @@ -0,0 +1,85 @@ +// RUN: %check_clang_tidy %s misc-suspicious-enum-usage %t -- -config="{CheckOptions: [{key: misc-suspicious-enum-usage.StrictMode, value: 0}]}" -- + +enum Empty { +}; + +enum A { + A = 1, + B = 2, + C = 4, + D = 8, + E = 16, + F = 32, + G = 63 +}; + +enum X { + X = 8, + Y = 16, + Z = 4 +}; + +enum { + P = 2, + Q = 3, + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask (contains mostly power-of-2 literals), but this literal is not a power-of-2 [misc-suspicious-enum-usage] + R = 4, + S = 8, + T = 16 +}; + +enum { + H, + I, + J, + K, + L +}; + +enum Days { + Monday, + Tuesday, + Wednesday, + Thursday, + Friday, + Saturday, + Sunday +}; + +Days bestDay() { + return Friday; +} + +int trigger() { + if (bestDay() | A) + return 1; + // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types + if (I | Y) + return 1; + // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types + unsigned p; + p = Q | P; +} + +int dont_trigger() { + if (A + G == E) + return 1; + else if ((Q | R) == T) + return 1; + else + int k = T | Q; + Empty EmptyVal; + int emptytest = EmptyVal | B; + int a = 1, b = 5; + int c = a + b; + int d = c | H, e = b * a; + a = B | C; + b = X | Z; + if (Tuesday != Monday + 1 || + Friday - Thursday != 1 || + Sunday + Wednesday == (Sunday | Wednesday)) + return 1; + if (H + I + L == 42) + return 1; + return 42; +}