szepet updated this revision to Diff 73157.
szepet marked 13 inline comments as done.
szepet added a comment.
Updates based on comments (the testfile note comments will be added in the next
commit)
Some changes in the algorithm/design:
In non-strict mode the checker will only investigate the operations between
different enum types.
In strict mode we check the suspicious bitmasks too.
(In the previous comments we always talked about only the strict mode
heuristics and looked on the strict results. )
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,92 @@
+// 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
+};
+
+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: :[[@LINE+1]]:1: warning: enum type seems like a bitmask (contains mostly power-of-2 literals) but some literal(s) are not a power-of-2
+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
+ 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;
+ // [LINE-1] 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-suspicious-enum-usage.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
@@ -0,0 +1,80 @@
+.. 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
@@ -80,6 +80,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,38 @@
+//===--- 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:
+ 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,238 @@
+//===--- 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 some literal(s) are not a 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(int NonPowOfTwoCounter, int EnumLen,
+ const ValueRange &VR, const EnumDecl *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::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.
+ 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:
+ // 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 = countNonPowOfTwoLiteralNum(EnumDec);
+ if (!isPossiblyBitMask(NonPowOfTwoCounter, EnumLen, VR, EnumDec))
+ return;
+ const auto *EnumDecExpr = dyn_cast<DeclRefExpr>(EnumExpr);
+ const auto *EnumConstDecl =
+ EnumDecExpr ? dyn_cast<EnumConstantDecl>(EnumDecExpr->getDecl())
+ : nullptr;
+
+ if (EnumConstDecl && isNonPowerOf2NorNullLiteral(EnumConstDecl)) {
+ diag(EnumConstDecl->getSourceRange().getBegin(), BitmaskErrorMessage);
+ diag(EnumExpr->getExprLoc(), BitmaskNoteMessage, DiagnosticIDs::Note);
+ } else if (!EnumConstDecl) {
+ diag(EnumDec->getInnerLocStart(), BitmaskVarErrorMessage);
+ diag(EnumExpr->getExprLoc(), BitmaskNoteMessage, DiagnosticIDs::Note);
+ }
+
+ return;
+ }
+
+ // Case 3:
+ // '|' or '+' operator where both argument comes from the same enum type
+ if (!StrictMode)
+ return;
+ 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;
+ bool RhsVar = !RhsConstant;
+
+ int NonPowOfTwoCounter = countNonPowOfTwoLiteralNum(EnumDec);
+ ValueRange VR(EnumDec);
+ int EnumLen = enumLength(EnumDec);
+ if (!isPossiblyBitMask(NonPowOfTwoCounter, EnumLen, VR, EnumDec))
+ return;
+ // Report left hand side parameter if neccessary.
+ if (LhsVar) {
+ diag(EnumDec->getInnerLocStart(), BitmaskVarErrorMessage);
+ diag(LhsExpr->getExprLoc(), BitmaskNoteMessage, DiagnosticIDs::Note);
+ } else if (isNonPowerOf2NorNullLiteral(LhsConstant)) {
+ diag(LhsConstant->getSourceRange().getBegin(), BitmaskErrorMessage);
+ diag(LhsExpr->getExprLoc(), BitmaskNoteMessage, DiagnosticIDs::Note);
+ }
+ // Report right hand side parameter if neccessary.
+ if (RhsVar) {
+ diag(EnumDec->getInnerLocStart(), BitmaskVarErrorMessage);
+ diag(RhsExpr->getExprLoc(), BitmaskNoteMessage, DiagnosticIDs::Note);
+ } else if (isNonPowerOf2NorNullLiteral(RhsConstant)) {
+ diag(RhsConstant->getSourceRange().getBegin(), BitmaskErrorMessage);
+ diag(LhsExpr->getExprLoc(), BitmaskNoteMessage, DiagnosticIDs::Note);
+ }
+}
+
+} // 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"
@@ -115,6 +116,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
