Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -8,6 +8,7 @@ BoolPointerImplicitConversionCheck.cpp DanglingHandleCheck.cpp DefinitionsInHeadersCheck.cpp + EnumMisuseCheck.cpp FoldInitTypeCheck.cpp ForwardDeclarationNamespaceCheck.cpp InaccurateEraseCheck.cpp Index: clang-tidy/misc/EnumMisuseCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/EnumMisuseCheck.h @@ -0,0 +1,39 @@ +//===--- EnumMisuseCheck.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_ENUM_MISUSE_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ENUM_MISUSE_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// 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 { +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; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ENUM_MISUSE_H Index: clang-tidy/misc/EnumMisuseCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/EnumMisuseCheck.cpp @@ -0,0 +1,231 @@ +//===--- EnumMisuseCheck.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 "EnumMisuseCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +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(); + MinVal = BeginVal; + MaxVal = BeginVal; + for (auto E : EnumDec->enumerators()) { + llvm::APSInt Val = E->getInitVal(); + if (Val < MinVal) + MinVal = Val; + else if (Val > MaxVal) + MaxVal = Val; + } + } +}; + +// 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(); +} + +bool isMaxValAllBitSet(const EnumDecl *EnumDec) { + for (auto I = EnumDec->enumerator_begin(), E = EnumDec->enumerator_end(); + I != E; ++I) { + auto It = I; + if (++It == E) + return I->getInitVal().countTrailingOnes() == + I->getInitVal().getActiveBits(); + } + llvm_unreachable("enum failed max value"); +} + +static int countNonPowOfTwoNum(const EnumDecl *EnumDec) { + int Counter = 0; + for (auto E : EnumDec->enumerators()) { + const llvm::APSInt Val = E->getInitVal(); + if (!Val.isPowerOf2() && Val.getExtValue()) + Counter++; + } + return Counter; +} + +// 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)))); + }; + + 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 EnumMisuseCheck::check(const MatchFinder::MatchResult &Result) { + + // 1. case: 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; + } + + // 2. case: + // 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 (!IsStrict) + return; + const auto *EnumDec = Result.Nodes.getNodeAs("enumDecl"); + ValueRange VR(EnumDec); + int EnumLen = enumLength(EnumDec); + int NonPowOfTwoCounter = countNonPowOfTwoNum(EnumDec); + if (isPossiblyBitField(NonPowOfTwoCounter, EnumLen, VR, EnumDec)) { + const auto *EnumDecExpr = dyn_cast(EnumExpr); + const auto *EnumConstDecl = + EnumDecExpr ? dyn_cast(EnumDecExpr->getDecl()) + : nullptr; + + if (EnumConstDecl && !(EnumConstDecl->getInitVal().isPowerOf2())) + diag(EnumExpr->getExprLoc(), "enum value is not power-of-2 unlike " + "most of the other values in the enum " + "type"); + else if (!EnumConstDecl) { + diag(EnumDec->getInnerLocStart(), "enum type seems like a bitfield but " + "possibly contains misspelled " + "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 + + 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 (!IsStrict && (LhsVar || RhsVar)) + return; + + int NonPowOfTwoCounter = countNonPowOfTwoNum(EnumDec); + ValueRange VR(EnumDec); + int EnumLen = enumLength(EnumDec); + if (isPossiblyBitField(NonPowOfTwoCounter, EnumLen, VR, EnumDec) && + (IsStrict || + (EnumBinOp->isBitwiseOp() && RhsConstant && LhsConstant && + hasCommonBit(LhsConstant->getInitVal(), RhsConstant->getInitVal())))) { + if (LhsVar) { + diag(EnumDec->getInnerLocStart(), "enum type seems like a bitfield but " + "possibly contains misspelled " + "number(s)"); + 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) { + diag(EnumDec->getInnerLocStart(), "enum type seems like a bitfield but " + "possibly contains misspelled " + "number(s)"); + 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 " + "unlike most other values in the enum type"); + } +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -17,6 +17,7 @@ #include "BoolPointerImplicitConversionCheck.h" #include "DanglingHandleCheck.h" #include "DefinitionsInHeadersCheck.h" +#include "EnumMisuseCheck.h" #include "FoldInitTypeCheck.h" #include "ForwardDeclarationNamespaceCheck.h" #include "InaccurateEraseCheck.h" @@ -72,6 +73,8 @@ "misc-dangling-handle"); CheckFactories.registerCheck( "misc-definitions-in-headers"); + CheckFactories.registerCheck( + "misc-enum-misuse"); CheckFactories.registerCheck( "misc-fold-init-type"); CheckFactories.registerCheck( Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -55,6 +55,7 @@ misc-bool-pointer-implicit-conversion misc-dangling-handle misc-definitions-in-headers + misc-enum-misuse misc-fold-init-type misc-forward-declaration-namespace misc-inaccurate-erase Index: docs/clang-tidy/checks/misc-enum-misuse.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-enum-misuse.rst @@ -0,0 +1,66 @@ +.. title:: clang-tidy - misc-enum-misuse + +misc-enum-misuse +================ + +The checker detects various cases when an enum is probably misused (as a bitfield). + 1. When "add" or "bitwise or" is used between two enum which come from different + types and these types valuerange's 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 and the non pow-of-2 numbers at most the third of the + enum type. (At the same time maximum 2 can be different and we assume that in + this case one of them can be the ALL options so the other is misspelled.) So + whenever it 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:: 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 =================================================================== --- /dev/null +++ test/clang-tidy/misc-enum-misuse-weak.cpp @@ -0,0 +1,88 @@ +// RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions: [{key: misc-enum-misuse.IsStrict, 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 +}; +// 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 +}; + +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 [misc-enum-misuse] + if (I | Y) + return 1; + // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types [misc-enum-misuse] + if (P + Q == R) + return 1; + // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: right hand side value is not power-of-2 unlike most other values in the enum type [misc-enum-misuse] + else if ((Q | R) == T) + return 1; + // CHECK-MESSAGES: :[[@LINE-2]]:13: warning: left hand side value is not power-of-2 unlike most other values in the enum type [misc-enum-misuse] + else + int k = T | Q; + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: right hand side value is not power-of-2 unlike most other values in the enum type [misc-enum-misuse] + unsigned p = R; + PP pp = Q; + p |= pp; + // Line 60 triggers the LINE:18 warning + p = A | G; + //p = G | X; + 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-enum-misuse.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-enum-misuse.cpp @@ -0,0 +1,81 @@ +// RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions: [{key: misc-enum-misuse.IsStrict, 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, + 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 [misc-enum-misuse] + if (I | Y) + return 1; + // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types [misc-enum-misuse] + unsigned 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() { + if (A + G == E) + return 1; + else if ((Q | R) == T) + return 1; + else + int k = T | Q; + return 0; + 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; +}