szepet updated this revision to Diff 65966.
szepet marked 12 inline comments as done.
szepet added a comment.
updates based on comments, counter and search functions replaced by std
functions
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-weak.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,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
+ 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;
+}
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
+ 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 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: 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 intervals 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: 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: 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,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,232 @@
+//===--- 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 *It1, const EnumConstantDecl *It2) {
+ return It1->getInitVal() < It2->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();
+}
+
+bool isMaxValAllBitSet(const EnumDecl *EnumDec) {
+
+ auto It = std::max_element(
+ EnumDec->enumerator_begin(), EnumDec->enumerator_end(),
+ [](const EnumConstantDecl *It1, const EnumConstantDecl *It2) {
+ return It1->getInitVal() < It2->getInitVal();
+ });
+ return It->getInitVal().countTrailingOnes() ==
+ It->getInitVal().getActiveBits();
+}
+
+static int countNonPowOfTwoNum(const EnumDecl *EnumDec) {
+ return std::count_if(EnumDec->enumerator_begin(), EnumDec->enumerator_end(),
+ [](const EnumConstantDecl *It) {
+ const llvm::APSInt Val = It->getInitVal();
+ return !Val.isPowerOf2() && Val.getExtValue();
+ });
+}
+
+// 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<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;
+ }
+
+ // 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<Expr>("enumExpr")) {
+ if (!IsStrict)
+ return;
+ const auto *EnumDec = Result.Nodes.getNodeAs<EnumDecl>("enumDecl");
+ ValueRange VR(EnumDec);
+ int EnumLen = enumLength(EnumDec);
+ int NonPowOfTwoCounter = countNonPowOfTwoNum(EnumDec);
+ if (isPossiblyBitField(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 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<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 ? cast<EnumConstantDecl>(LhsDecExpr->getDecl()) : nullptr;
+ const auto *RhsConstant =
+ RhsDecExpr ? cast<EnumConstantDecl>(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/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
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
