szepet updated this revision to Diff 70324.
szepet marked 4 inline comments as done.
szepet added a comment.
cast to dyn-cast change in order to fix a bug, changes based on comments
https://reviews.llvm.org/D22507
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/EnumMisuseCheck.cpp
clang-tidy/misc/EnumMisuseCheck.h
clang-tidy/misc/MiscTidyModule.cpp
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-enum-misuse.rst
test/clang-tidy/misc-enum-misuse-strict.cpp
test/clang-tidy/misc-enum-misuse.cpp
Index: test/clang-tidy/misc-enum-misuse.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/misc-enum-misuse.cpp
@@ -0,0 +1,85 @@
+// 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;
+ 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-enum-misuse-strict.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/misc-enum-misuse-strict.cpp
@@ -0,0 +1,92 @@
+// 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: 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: 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: 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<DefinitionsInHeadersCheck>(
"misc-definitions-in-headers");
+ CheckFactories.registerCheck<EnumMisuseCheck>(
+ "misc-enum-misuse");
CheckFactories.registerCheck<FoldInitTypeCheck>(
"misc-fold-init-type");
CheckFactories.registerCheck<ForwardDeclarationNamespaceCheck>(
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,229 @@
+//===--- 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 <algorithm>
+
+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<BinaryOperator>("diffEnumOp")) {
+ const auto *EnumDec = Result.Nodes.getNodeAs<EnumDecl>("enumDecl");
+ const auto *OtherEnumDec =
+ Result.Nodes.getNodeAs<EnumDecl>("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<Expr>("enumExpr")) {
+ if (!StrictMode)
+ return;
+ const auto *EnumDec = Result.Nodes.getNodeAs<EnumDecl>("enumDecl");
+ ValueRange VR(EnumDec);
+ int EnumLen = enumLength(EnumDec);
+ int NonPowOfTwoCounter = countNonPowOfTwoNum(EnumDec);
+ if (isPossiblyBitMask(NonPowOfTwoCounter, EnumLen, VR, EnumDec)) {
+ const auto *EnumDecExpr = dyn_cast<DeclRefExpr>(EnumExpr);
+ const auto *EnumConstDecl =
+ EnumDecExpr ? dyn_cast<EnumConstantDecl>(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<BinaryOperator>("enumBinOp");
+
+ const auto *EnumDec = Result.Nodes.getNodeAs<EnumDecl>("enumDecl");
+
+ const auto *LhsExpr = Result.Nodes.getNodeAs<Expr>("lhsExpr");
+ const auto *RhsExpr = Result.Nodes.getNodeAs<Expr>("rhsExpr");
+
+ const auto *LhsDecExpr = dyn_cast<DeclRefExpr>(LhsExpr);
+ const auto *RhsDecExpr = dyn_cast<DeclRefExpr>(RhsExpr);
+
+ const auto *LhsConstant =
+ LhsDecExpr ? dyn_cast<EnumConstantDecl>(LhsDecExpr->getDecl()) : nullptr;
+ const auto *RhsConstant =
+ RhsDecExpr ? dyn_cast<EnumConstantDecl>(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/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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
