Index: clang-tidy/misc/EnumMisuseCheck.h =================================================================== --- clang-tidy/misc/EnumMisuseCheck.h +++ clang-tidy/misc/EnumMisuseCheck.h @@ -16,18 +16,20 @@ namespace tidy { namespace misc { -/// FIXME: Write a short description. -/// +/// The checker detects various cases when an enum is probably misused (as a +/// bitfield). /// For the user-facing documentation see: /// http://clang.llvm.org/extra/clang-tidy/checks/misc-enum-misuse.html class EnumMisuseCheck : public ClangTidyCheck { - const bool IsStrict; +private: + const bool IsStrict; - public: - EnumMisuseCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context), IsStrict(Options.get("IsStrict", 1)) {} - void registerMatchers(ast_matchers::MatchFinder *Finder) override; - void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +public: + EnumMisuseCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), IsStrict(Options.get("IsStrict", 1)) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; }; } // namespace misc Index: clang-tidy/misc/EnumMisuseCheck.cpp =================================================================== --- clang-tidy/misc/EnumMisuseCheck.cpp +++ clang-tidy/misc/EnumMisuseCheck.cpp @@ -16,23 +16,12 @@ namespace tidy { namespace misc { -// Return the number of EnumConstantDecls in an EnumDecl. -static int enumLen(const EnumDecl *EnumDec) { - int Counter = 0; - - for (auto E : EnumDec->enumerators()) { - (void)E; - Counter++; - } - return Counter; -} - // Stores a min and a max value which describe an interval. struct ValueRange { - llvm::APSInt MinVal, MaxVal; + llvm::APSInt MinVal; + llvm::APSInt MaxVal; ValueRange(const EnumDecl *EnumDec) { - llvm::APSInt BeginVal = EnumDec->enumerator_begin()->getInitVal(); MinVal = BeginVal; MaxVal = BeginVal; @@ -46,19 +35,24 @@ } }; +// 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)); } -bool hasCommonBit(const llvm::APSInt &Val1, const llvm::APSInt &Val2) { +static bool hasCommonBit(const llvm::APSInt &Val1, const llvm::APSInt &Val2) { return (Val1 & Val2).getExtValue(); } bool isMaxValAllBitSet(const EnumDecl *EnumDec) { for (auto I = EnumDec->enumerator_begin(), E = EnumDec->enumerator_end(); - I != E; I++) { + I != E; ++I) { auto It = I; if (++It == E) return I->getInitVal().countTrailingOnes() == @@ -77,8 +71,23 @@ return Counter; } -void EnumMisuseCheck::registerMatchers(MatchFinder *Finder) { +// We check if there is at most 2 not power-of-2 value in the enum type and the +// it contains enough element to make sure it could be a biftield, but we +// exclude the cases when the last number is the sum of the lesser values or +// when it could contain consecutive numbers. +static bool isPossiblyBitField(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 && isMaxValAllBitSet(EnumDec)); +} +void EnumMisuseCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "IsStrict", IsStrict); +} + +void EnumMisuseCheck::registerMatchers(MatchFinder *Finder) { const auto enumExpr = [](StringRef RefName, StringRef DeclName) { return allOf(ignoringImpCasts(expr().bind(RefName)), ignoringImpCasts(hasType(enumDecl().bind(DeclName)))); @@ -113,19 +122,12 @@ hasRHS(enumExpr("enumExpr", "enumDecl"))), this); } -// if there is only one not power-of-2 value in the enum unless it is -static bool isPossiblyBitField(const int NonPowOfTwoCounter, const int EnumLen, - const ValueRange &VR, const EnumDecl *EnumDec) { - return NonPowOfTwoCounter >= 1 && NonPowOfTwoCounter <= 2 && NonPowOfTwoCounter < enumLen(EnumDec) / 2 && - (VR.MaxVal - VR.MinVal != EnumLen - 1) && !(NonPowOfTwoCounter == 1 && isMaxValAllBitSet(EnumDec)); -} void EnumMisuseCheck::check(const MatchFinder::MatchResult &Result) { - const auto *DiffEnumOp = Result.Nodes.getNodeAs("diffEnumOp"); // 1. case: The two enum values come from different types. - if (DiffEnumOp) { - + if (const auto *DiffEnumOp = + Result.Nodes.getNodeAs("diffEnumOp")) { const auto *EnumDec = Result.Nodes.getNodeAs("enumDecl"); const auto *OtherEnumDec = Result.Nodes.getNodeAs("otherEnumDecl"); @@ -137,7 +139,6 @@ if (!hasDisjointValueRange(EnumDec, OtherEnumDec)) diag(DiffEnumOp->getOperatorLoc(), "enum values are from different enum types"); - return; } @@ -145,14 +146,13 @@ // a, Investigating the right hand side of += or |= operator. // b, When the operator is | or + but only one of them is an // EnumExpr - const auto *EnumExpr = Result.Nodes.getNodeAs("enumExpr"); - if (EnumExpr) { + if (const auto *EnumExpr = Result.Nodes.getNodeAs("enumExpr")) { if (!IsStrict) return; const auto *EnumDec = Result.Nodes.getNodeAs("enumDecl"); ValueRange VR(EnumDec); - int EnumLen = enumLen(EnumDec); + int EnumLen = enumLength(EnumDec); int NonPowOfTwoCounter = countNonPowOfTwoNum(EnumDec); if (isPossiblyBitField(NonPowOfTwoCounter, EnumLen, VR, EnumDec)) { const auto *EnumDecExpr = dyn_cast(EnumExpr); @@ -167,13 +167,14 @@ else if (!EnumConstDecl) { diag(EnumDec->getInnerLocStart(), "enum type seems like a bitfield but " "possibly contains misspelled " - "number(s) "); + "number(s)"); diag(EnumExpr->getExprLoc(), "Used here as a bitfield.", DiagnosticIDs::Note); } } return; } + // 3. case // | or + operator where both argument comes from the same enum type @@ -198,7 +199,7 @@ int NonPowOfTwoCounter = countNonPowOfTwoNum(EnumDec); ValueRange VR(EnumDec); - int EnumLen = enumLen(EnumDec); + int EnumLen = enumLength(EnumDec); if (isPossiblyBitField(NonPowOfTwoCounter, EnumLen, VR, EnumDec) && (IsStrict || (EnumBinOp->isBitwiseOp() && RhsConstant && LhsConstant && @@ -207,19 +208,17 @@ diag(EnumDec->getInnerLocStart(), "enum type seems like a bitfield but " "possibly contains misspelled " "number(s)"); - diag(EnumExpr->getExprLoc(), "Used here as a bitfield.", + diag(LhsExpr->getExprLoc(), "Used here as a bitfield.", DiagnosticIDs::Note); } else if (!(LhsConstant->getInitVal()).isPowerOf2()) diag(LhsExpr->getExprLoc(), "left hand side value is not power-of-2 " "unlike most other values in the enum type"); if (RhsVar) { - // if (LhsVar) - // return; diag(EnumDec->getInnerLocStart(), "enum type seems like a bitfield but " "possibly contains misspelled " "number(s)"); - diag(EnumExpr->getExprLoc(), "Used here as a bitfield.", + diag(RhsExpr->getExprLoc(), "Used here as a bitfield.", DiagnosticIDs::Note); } else if (!(RhsConstant->getInitVal()).isPowerOf2()) diag(RhsExpr->getExprLoc(), "right hand side value is not power-of-2 " Index: docs/clang-tidy/checks/misc-enum-misuse.rst =================================================================== --- docs/clang-tidy/checks/misc-enum-misuse.rst +++ docs/clang-tidy/checks/misc-enum-misuse.rst @@ -23,3 +23,44 @@ enum val. (only in "Strict") 4. Check both side of | or + operator where the enum values are from the same enum type. + +Examples: + +.. code:: c++ + +//1. +Enum {A, B, C} +Enum {D, E, F} +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 + + + +//2. + +Enum Bitfield { A = 0; + B = 1; + C = 2; + D = 4; + E = 8; + F = 16; + G = 31; //OK, real bitfield +} + +Enum AlmostBitfield { 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 bitfield + +void f(const string&); // Good: const is not top level. Index: test/clang-tidy/misc-enum-misuse-weak.cpp =================================================================== --- test/clang-tidy/misc-enum-misuse-weak.cpp +++ test/clang-tidy/misc-enum-misuse-weak.cpp @@ -6,24 +6,28 @@ D = 8, E = 16, F = 32, - G = 63 }; + G = 63 +}; enum X { X = 8, Y = 16, - Z = 4 }; + Z = 4 +}; // CHECK-MESSAGES: :[[@LINE+1]]:1: warning: enum type seems like a bitfield but possibly contains misspelled number(s) [misc-enum-misuse] enum PP { P = 2, Q = 3, R = 4, S = 8, T = 16, - U = 31}; + U = 31 +}; enum { H, I, J, K, - L }; + L +}; enum Days { Monday, Tuesday, @@ -31,7 +35,8 @@ Thursday, Friday, Saturday, - Sunday }; + Sunday +}; Days bestDay() { return Friday; @@ -59,7 +64,7 @@ // Line 60 triggers the LINE:18 warning p = A | G; //p = G | X; - return 0; + return 0; } int dont_trigger() { @@ -68,7 +73,7 @@ int d = c | H, e = b * a; a = B | C; b = X | Z; - + unsigned bitflag; enum A aa = B; bitflag = aa | C; @@ -80,5 +85,4 @@ if (H + I + L == 42) return 1; return 42; -}; - +} Index: test/clang-tidy/misc-enum-misuse.cpp =================================================================== --- test/clang-tidy/misc-enum-misuse.cpp +++ test/clang-tidy/misc-enum-misuse.cpp @@ -1,6 +1,7 @@ // RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions: [{key: misc-enum-misuse.IsStrict, value: 0}]}" -- -enum Empty {}; +enum Empty { +}; enum A { A = 1, B = 2, @@ -8,23 +9,27 @@ D = 8, E = 16, F = 32, - G = 63 }; + G = 63 +}; enum X { X = 8, Y = 16, - Z = 4 }; + Z = 4 +}; -enum {P = 2, - Q = 3, - R = 4, - S = 8, - T = 16 }; +enum { P = 2, + Q = 3, + R = 4, + S = 8, + T = 16 +}; enum { H, I, J, K, - L }; + L +}; enum Days { Monday, Tuesday, @@ -32,14 +37,14 @@ Thursday, Friday, Saturday, - Sunday }; + 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 [misc-enum-misuse] @@ -47,9 +52,8 @@ return 1; // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types [misc-enum-misuse] unsigned p; - p = Q | P; + p = Q | P; // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: left hand side value is not power-of-2 unlike most other values in the enum type [misc-enum-misuse] - } int dont_trigger() { @@ -74,4 +78,4 @@ if (H + I + L == 42) return 1; return 42; -}; +}