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,38 @@ +//===--- 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 +/// bitmask). +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-enum-misuse.html +class EnumMisuseCheck : public ClangTidyCheck { +public: + EnumMisuseCheck(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; +}; + +} // 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,228 @@ +//===--- 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" +#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 isMaxValAllBitSet(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(); + }); + return EnumConst->getInitVal().countTrailingOnes() == + EnumConst->getInitVal().getActiveBits(); +} + +static int countNonPowOfTwoNum(const EnumDecl *EnumDec) { + return std::count_if(EnumDec->enumerator_begin(), EnumDec->enumerator_end(), + [](const EnumConstantDecl *E) { + const llvm::APSInt Val = E->getInitVal(); + return !Val.isPowerOf2() && Val.getExtValue(); + }); +} + +/// Check if there are two or more enumerators that are not a power of 2 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 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 && isMaxValAllBitSet(EnumDec)); +} + +EnumMisuseCheck::EnumMisuseCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), StrictMode(Options.get("StrictMode", 1)) {} + +void EnumMisuseCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "StrictMode", StrictMode); +} + +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) { + // 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 = countNonPowOfTwoNum(EnumDec); + if (isPossiblyBitMask(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 bitmask but " + "possibly contains misspelled " + "number(s)"); + diag(EnumExpr->getExprLoc(), "used here as a bitmask", + 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 ? cast(LhsDecExpr->getDecl()) : nullptr; + const auto *RhsConstant = + RhsDecExpr ? cast(RhsDecExpr->getDecl()) : nullptr; + bool LhsVar = !LhsConstant, RhsVar = !RhsConstant; + + if (!StrictMode && (LhsVar || RhsVar)) + return; + + int NonPowOfTwoCounter = countNonPowOfTwoNum(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(), "enum type seems like a bitmask but " + "possibly contains misspelled " + "number(s)"); + diag(LhsExpr->getExprLoc(), "used here as a bitmask", + 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 bitmask but " + "possibly contains misspelled " + "number(s)"); + diag(RhsExpr->getExprLoc(), "used here as a bitmask", + 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 @@ -57,6 +57,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-argument-comment.rst =================================================================== --- docs/clang-tidy/checks/misc-argument-comment.rst +++ docs/clang-tidy/checks/misc-argument-comment.rst @@ -24,5 +24,5 @@ .. option:: StrictMode - When non-zero, the check will ignore leading and trailing underscores and + When non-zero, the check will not ignore leading and trailing underscores and case when comparing parameter names. Index: docs/clang-tidy/checks/misc-enum-misuse.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-enum-misuse.rst @@ -0,0 +1,67 @@ +.. title:: clang-tidy - misc-enum-misuse + +misc-enum-misuse +================ + +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++ + + // Case 1 (strict and weak mode): + 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-enum-misuse-strict.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-enum-misuse-strict.cpp @@ -0,0 +1,87 @@ +// RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions: [{key: misc-enum-misuse.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 +}; +// CHECK-MESSAGES: :[[@LINE+1]]:1: warning: enum type seems like a bitmask 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 + if (I | Y) + return 1; + // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types + 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 + 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 + 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 + unsigned p = R; + PP pp = Q; + p |= pp; + // Line 65 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-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.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, + 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 + 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 +} + +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; +}