szepet updated this revision to Diff 82488. szepet marked 6 inline comments as done. szepet added a comment.
The requested changes have been made. Some more refactor on the Case2 since it is the same as the LHS/RHS case. Moved more common statements out of the branch (Case2-3) for better readabilty. (And less code duplication.) https://reviews.llvm.org/D22507 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp clang-tidy/misc/SuspiciousEnumUsageCheck.cpp clang-tidy/misc/SuspiciousEnumUsageCheck.h docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/misc-suspicious-enum-usage.rst test/clang-tidy/misc-suspicious-enum-usage-strict.cpp test/clang-tidy/misc-suspicious-enum-usage.cpp
Index: test/clang-tidy/misc-suspicious-enum-usage.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-suspicious-enum-usage.cpp @@ -0,0 +1,90 @@ +// 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, + 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() { + Empty EmptyVal; + int emptytest = EmptyVal | B; + 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 +} + +int dont_trigger() { + unsigned p; + p = Q | P; + + 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; +} Index: test/clang-tidy/misc-suspicious-enum-usage-strict.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-suspicious-enum-usage-strict.cpp @@ -0,0 +1,98 @@ +// 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 +}; + +// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: enum type seems like a bitmask (contains mostly power-of-2 literals) but a literal is not power-of-2 +// CHECK-MESSAGES: :76:7: note: used here as a bitmask +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: :70:13: note: used here as a bitmask +}; +// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: enum type seems like a bitmask (contains mostly power-of-2 literals) but some literals are not power-of-2 +// CHECK-MESSAGES: :73:8: note: used here as a bitmask +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 + // CHECK-MESSAGES: :65:11: note: used here as a bitmask + 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; + + enum X x = Z; + p = x | ZZ; + 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: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-suspicious-enum-usage.rst @@ -0,0 +1,78 @@ +.. 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. + +The following cases will be investigated only using :option:`StrictMode`. We +regard the enum as a (suspicious) +bitmask if the three conditions below are true at the same time: + +* at most half of the elements of the enum are non pow-of-2 numbers (because of + short enumerations) +* there is another non pow-of-2 number than the enum constant representing all + choices (the result "bitwise OR" operation of all enum elements) +* enum type variable/enumconstant is used as an argument of a `+` or "bitwise OR + " operator + +So whenever the non pow-of-2 element is used as a bitmask element we diagnose a +misuse and give a warning. + +2. Investigating the right hand side of `+=` and `|=` operator. +3. Check only the enum value side of a `|` and `+` operator if one of them is not + enum val. +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: + 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. + +Options +------- +.. option:: StrictMode + + Default value: 0. + When non-null the suspicious bitmask usage will be investigated additionally + to the different enum usage check. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -81,6 +81,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: clang-tidy/misc/SuspiciousEnumUsageCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/SuspiciousEnumUsageCheck.h @@ -0,0 +1,39 @@ +//===--- 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: + void checkSuspiciousBitmaskUsage(const Expr*, const EnumDecl*); + const bool StrictMode; +}; + +} // 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,216 @@ +//===--- 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 <algorithm> + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +static const char DifferentEnumErrorMessage[] = + "enum values are from different enum types"; + +static const char BitmaskErrorMessage[] = + "enum type seems like a bitmask (contains mostly " + "power-of-2 literals), but this literal is not a " + "power-of-2"; + +static const char BitmaskVarErrorMessage[] = + "enum type seems like a bitmask (contains mostly " + "power-of-2 literals) but %plural{1:a literal is|:some literals are}0 not " + "power-of-2"; + +static const char BitmaskNoteMessage[] = "used here as a bitmask"; + +/// Stores a min and a max value which describe an interval. +struct ValueRange { + llvm::APSInt MinVal; + llvm::APSInt MaxVal; + + ValueRange(const EnumDecl *EnumDec) { + 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 isNonPowerOf2NorNullLiteral(const EnumConstantDecl *EnumConst) { + llvm::APSInt Val = EnumConst->getInitVal(); + if (Val.isPowerOf2() || !Val.getBoolValue()) + return false; + const Expr *InitExpr = EnumConst->getInitExpr(); + if (!InitExpr) + return true; + return isa<IntegerLiteral>(InitExpr->IgnoreImpCasts()); +} + +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() && + isa<IntegerLiteral>(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(const EnumDecl *EnumDec) { + ValueRange VR(EnumDec); + int EnumLen = enumLength(EnumDec); + int NonPowOfTwoCounter = countNonPowOfTwoLiteralNum(EnumDec); + return NonPowOfTwoCounter >= 1 && NonPowOfTwoCounter <= 2 && + NonPowOfTwoCounter < EnumLen / 2 && + (VR.MaxVal - VR.MinVal != EnumLen - 1) && + !(NonPowOfTwoCounter == 1 && isMaxValAllBitSetLiteral(EnumDec)); +} + +SuspiciousEnumUsageCheck::SuspiciousEnumUsageCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), StrictMode(Options.get("StrictMode", 0)) {} + +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"))))))), + 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::checkSuspiciousBitmaskUsage( + const Expr *NodeExpr, const EnumDecl *EnumDec) { + const auto *EnumExpr = dyn_cast<DeclRefExpr>(NodeExpr); + const auto *EnumConst = + EnumExpr ? dyn_cast<EnumConstantDecl>(EnumExpr->getDecl()) : nullptr; + + // Report the parameter if neccessary. + if (!EnumConst) { + diag(EnumDec->getInnerLocStart(), BitmaskVarErrorMessage) + << countNonPowOfTwoLiteralNum(EnumDec); + diag(EnumExpr->getExprLoc(), BitmaskNoteMessage, DiagnosticIDs::Note); + } else if (isNonPowerOf2NorNullLiteral(EnumConst)) { + diag(EnumConst->getSourceRange().getBegin(), BitmaskErrorMessage); + diag(EnumExpr->getExprLoc(), BitmaskNoteMessage, DiagnosticIDs::Note); + } +} + +void SuspiciousEnumUsageCheck::check(const MatchFinder::MatchResult &Result) { + // Case 1: The two enum values come from different types. + if (const auto *DiffEnumOp = + Result.Nodes.getNodeAs<BinaryOperator>("diffEnumOp")) { + const auto *EnumDec = Result.Nodes.getNodeAs<EnumDecl>("enumDecl"); + const auto *OtherEnumDec = + Result.Nodes.getNodeAs<EnumDecl>("otherEnumDecl"); + // Skip when one of the parameters is an empty enum. The + // hasDisjointValueRange function could not decide the values properly in + // case of an empty enum. + if (EnumDec->enumerator_begin() == EnumDec->enumerator_end() || + OtherEnumDec->enumerator_begin() == OtherEnumDec->enumerator_end()) + return; + + if (!hasDisjointValueRange(EnumDec, OtherEnumDec)) + diag(DiffEnumOp->getOperatorLoc(), DifferentEnumErrorMessage); + return; + } + + // Case 2 and 3 only checked in strict mode. The checker tries to detect + // suspicious bitmasks which contains values initialized by non power-of-2 + // literals. + if (!StrictMode) + return; + const auto *EnumDec = Result.Nodes.getNodeAs<EnumDecl>("enumDecl"); + if (!isPossiblyBitMask(EnumDec)) + 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<Expr>("enumExpr")) { + checkSuspiciousBitmaskUsage(EnumExpr, EnumDec); + return; + } + + // Case 3: + // '|' or '+' operator where both argument comes from the same enum type + const auto *LhsExpr = Result.Nodes.getNodeAs<Expr>("lhsExpr"); + checkSuspiciousBitmaskUsage(LhsExpr, EnumDec); + + const auto *RhsExpr = Result.Nodes.getNodeAs<Expr>("rhsExpr"); + checkSuspiciousBitmaskUsage(RhsExpr, EnumDec); +} + +} // namespace misc +} // namespace tidy +} // namespace clang 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" @@ -116,6 +117,8 @@ "misc-string-integer-assignment"); CheckFactories.registerCheck<StringLiteralWithEmbeddedNulCheck>( "misc-string-literal-with-embedded-nul"); + CheckFactories.registerCheck<SuspiciousEnumUsageCheck>( + "misc-suspicious-enum-usage"); CheckFactories.registerCheck<SuspiciousMissingCommaCheck>( "misc-suspicious-missing-comma"); CheckFactories.registerCheck<SuspiciousSemicolonCheck>( 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
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits