Clang-tidy - Enum misuse check

[szepet]

Summary

The checker detects various cases when an enum is probably misused (as a bitmask).

For the heuristics suggestions and improvement ideas are welcome.

[hokein]

Please make sure the code align with LLVM code style (clang-format can help you to that).

clang-tidy/misc/EnumMisuseCheck.cpp

20 ↗

(On Diff #64464)

You can use std::distance(Enum->enumerator_begin(), Enum->enumerator_end()).

32 ↗

(On Diff #64464)

One variable declaration per line.

61 ↗

(On Diff #64464)

LLVM code style prefers to use preincrement (++I), the same below.

217 ↗

(On Diff #64464)

An unrelated comment.

clang-tidy/misc/EnumMisuseCheck.h

19 ↗

(On Diff #64464)

Fix FIXME.

24 ↗

(On Diff #64464)

Put it to private member.

28 ↗

(On Diff #64464)

As you defined a IsStrict option in the check, you need to override storeOptions method.

void EnumMisuseCheck::storeOptions(
    ClangTidyOptions::OptionMap &Opts) {
  Options.store(Opts, "IsStrict", IsStrict);
}

docs/clang-tidy/checks/misc-enum-misuse.rst

4 ↗

(On Diff #64464)

Could you add a few code examples here?

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst. See pre-4.0 branch versions as example.

clang-tidy/misc/EnumMisuseCheck.cpp

116 ↗

(On Diff #64464)

Please separate with empty line.

test/clang-tidy/misc-enum-misuse-weak.cpp

9 ↗

(On Diff #64464)

Please put closing }; on next line. Same in other places.

83 ↗

(On Diff #64464)

Please fix -Wextra-semi. Is next empty line excessive?

test/clang-tidy/misc-enum-misuse.cpp

42 ↗

(On Diff #64464)

Unnecessary empty line. Same at the end of function.

77 ↗

(On Diff #64464)

Please fix -Wextra-semi.

[etienneb]

drive-by, some comments.
Thanks for the check

clang-tidy/misc/EnumMisuseCheck.cpp

20 ↗

(On Diff #64464)

We tend to keep name meaningful and avoid abbreviation.
enumLen -> enumLength

35 ↗

(On Diff #64464)

nit: remove empty line

55 ↗

(On Diff #64464)

nit: static

81 ↗

(On Diff #64464)

nit: remove empty line

117 ↗

(On Diff #64464)

nit: const int x --> int x for all parameters

119 ↗

(On Diff #64464)

nit: line too long

127 ↗

(On Diff #64464)

A common pattern is:

if (const auto *DiffEnumOp = Result.Nodes.getNodeAs<BinaryOperator>("diffEnumOp")) {
   [...]
}

This way the scope is smaller and there is less chance to use it by mistake.

128 ↗

(On Diff #64464)

nit: no empty line

150 ↗

(On Diff #64464)

ditto for th "if (var = ...)" pattern

170 ↗

(On Diff #64464)

nit: remove the space after (s)

177 ↗

(On Diff #64464)

To be consistent with your comments, add newline here.

[szepet]

updating patch based on review comments

[aaron.ballman]

clang-tidy/misc/EnumMisuseCheck.cpp

31 ↗

(On Diff #64464)

I think this class can be replaced by std::minmax_element() over the EnumDec->enumerators() and then grabbing the actual values out of the resulting std::pair.

59 ↗

(On Diff #64464)

This function doesn't do what is described. It appears to be checking if the last value in the enumeration has all its bits set, not if the max value has all the bits set. e.g., it won't check:

enum E {
  MaxValue = 0xFFFFFFFF,
  First = 0,
  Second
};

clang-tidy/misc/EnumMisuseCheck.h

24 ↗

(On Diff #64464)

It is a private member already. I think this usage is fine.

[hokein]

Seems that you only uploaded the diff part, I only see the updated part of your patch now, and can't see the whole patch now (The review page says "Context not available"), could you upload the whole patch again?

docs/clang-tidy/checks/misc-enum-misuse.rst

31 ↗

(On Diff #64897)

Could you add some descriptions about what 1 stands for here? strict or non-strict? please leave a blank between "//" and comment words, the same below. Make sure the code here align with code style.

62 ↗

(On Diff #64897)

extra indentations.

[szepet]

full diff submitted

[etienneb]

some nits

docs/clang-tidy/checks/misc-enum-misuse.rst

37 ↗

(On Diff #65310)

nit: space after //
here and below.

37 ↗

(On Diff #65310)

nit: intervalls (typo)

test/clang-tidy/misc-enum-misuse-weak.cpp

48 ↗

(On Diff #65310)

you can remove "[misc-enum-misuse]" from the following line.
Only the first occurrence is enough.

[Prazek]

clang-tidy/misc/EnumMisuseCheck.cpp

19 ↗

(On Diff #65310)

s/\/\//\/\/\/

In other words, change to / (doxygen comment)

192 ↗

(On Diff #65310)

If the pointer returned from dyn_cast<EnumConstantDecl>(LhsDeclExpr->getDecl()) is assumed to be always not null, thn you can change it to cast<> (It will assert if null instead of returning nullptr)

docs/clang-tidy/checks/misc-enum-misuse.rst

29 ↗

(On Diff #65310)

I think all the code here needs to be indented by one or more to get into ..code:: block.

[szepet]

updates based on comments, counter and search functions replaced by std functions

[aaron.ballman]

clang-tidy/misc/EnumMisuseCheck.cpp

40 ↗

(On Diff #65966)

Doxygen comment here as well.

75 ↗

(On Diff #65966)

The wrapping for this comment is a bit strange, also should be using doxygen-style comments. Also, the grammar is a bit off for the comment. I would recommend:

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.

Also, I would call this isPossiblyBitMask instead of using "bit field" because a bit-field is a syntactic construct that is unrelated. Bitmask types are covered in the C++ standard under [bitmask.types] and are slightly different, but more closely related to what this check is looking for.

209 ↗

(On Diff #65966)

I think this diagnostic text is a bit confusing. The enum type shouldn't seem like a bit-field (but more like a bitmask), and I'm not certain what a misspelled number would be. I think the diagnostic is effectively saying that we guess this might be a bitmask, but some of the enumerator values don't make sense for that guess, and so this may be suspicious code -- but I really worry about the false positive rate for such a diagnostic. Have you run this check over a large body of work (like LLVM, Firefox, Chrome, etc)? If so, how do the diagnostics look?

Perhaps a different way to word the diagnostic is: "enum type used as a bitmask with an enumerator value that is not a power of 2" and place the diagnostic on the enumerator(s) that cause the problem rather than the enumeration as a whole.

212 ↗

(On Diff #65966)

s/bitmask/bitfield

214 ↗

(On Diff #65966)

Are these spurious parens around LhsConstant->getInitVal()?

219 ↗

(On Diff #65966)

Same comments here as above; if you can remove the duplicate code, that would be great, too.

clang-tidy/misc/EnumMisuseCheck.h

24 ↗

(On Diff #65310)

Can remove the private access specifier -- it's already private.

test/clang-tidy/misc-enum-misuse-weak.cpp

1 ↗

(On Diff #65966)

Please run clang-format over this file.

test/clang-tidy/misc-enum-misuse.cpp

1 ↗

(On Diff #65966)

Please run clang-format over this file.

[alexfh]

clang-tidy/misc/EnumMisuseCheck.cpp

28 ↗

(On Diff #65966)

I don't think the compound statement here is needed.

48 ↗

(On Diff #65966)

Please remove the outermost parentheses, they are superfluous.

56 ↗

(On Diff #65966)

nit: Please remove the empty line.

128 ↗

(On Diff #65966)

Please remove the empty line.

146 ↗

(On Diff #65966)

Formatting of the list seems rather uncommon for English text. Let's change to:

// Case 2:
//   a. ......
//   b. ......

(note the period after a and b, and Case N instead of 1. case).

164 ↗

(On Diff #65966)

No parentheses needed after !.

164 ↗

(On Diff #65966)

Braces should be used consistently in each if-else chain. Please add braces around the first if body.

172 ↗

(On Diff #65966)

No capitalization and trailing period needed.

224 ↗

(On Diff #65966)

Inner parentheses are not needed.

[alexfh]

clang-tidy/misc/EnumMisuseCheck.cpp

59 ↗

(On Diff #65966)

nit: The parameters are not iterators, so I'd change It1 and It2 to EnumConst1 and EnumConst2, E1 and E2, First and Second, Left and Right or something else not related to iterators. Same above in ValueRange and below in countNonPowOfTwoNum.

clang-tidy/misc/EnumMisuseCheck.h

25 ↗

(On Diff #65310)

I'd move the private section to the bottom of the class definition.

[szepet]

Changes based on comments.

clang-tidy/misc/EnumMisuseCheck.cpp

75 ↗

(On Diff #65966)

Thanks for the recommendations! As you can see my grammar and vocabulary is a "bit strange" so I really appreciated the correction!

209 ↗

(On Diff #65966)

Here you can see the results on LLVM. (weak options, less false positive)

error.txt2 KBDownload

Here I have to mention that the last 4 results could be combined into one, because it`s actually the usage of the same enum in different files. If you wish I could easily change it.

[alexfh]

clang-tidy/misc/EnumMisuseCheck.cpp

215 ↗

(On Diff #68968)

Diagnostic messages are not full sentences, so they shouldn't start with a capital letter and end with a period.

clang-tidy/misc/EnumMisuseCheck.h

26 ↗

(On Diff #68968)

1. Please move the constructor body to the .cpp file so that the code reading and storing options are together.
2. Let's use a global flag StrictMode (used by one more check currently: http://clang.llvm.org/extra/clang-tidy/checks/misc-argument-comment.html, clang-tidy/misc/ArgumentCommentCheck.cpp). It can still be configured separately for each check, but overall it improves consistency. Also, let's make it non-strict by default.
   
   Options.get(...)

should change to

StrictMode(Options.getLocalOrGlobal("StrictMode", 0) != 0)

docs/clang-tidy/checks/misc-enum-misuse.rst

21 ↗

(On Diff #68968)

Please enclose inline code snippets in backquotes (+=, |=, etc.). Many places in this file and in doxygen comments.

29 ↗

(On Diff #68968)

Should be .. code-block:: c++.

31 ↗

(On Diff #68968)

Code block should be indented. Please compile the doc and make sure the result seems reasonable.

31 ↗

(On Diff #68968)

Could you add some descriptions about what 1 stands for here? strict or non-strict? please leave a blank between "//" and comment words, the same below. Make sure the code here align with code style.

Still not addressed.

38 ↗

(On Diff #68968)

nit: space after //
here and below.

This is still not addressed.

test/clang-tidy/misc-enum-misuse-weak.cpp

2 ↗

(On Diff #68968)

The format still seems off.

66 ↗

(On Diff #68968)

Is the commented line needed?

test/clang-tidy/misc-enum-misuse.cpp

2 ↗

(On Diff #68968)

Doesn't seem to be done: the format is still off.

[szepet]

Changes based on comments, fix a cast to dyn_cast bug, description updated (hopefully it became more clear).

[szepet]

test/clang-tidy/misc-enum-misuse.cpp

3 ↗

(On Diff #70017)

Could you specify which part of the file seems off? I have run the clang format with the same options on testfiles as on the others.

[aaron.ballman]

test/clang-tidy/misc-enum-misuse.cpp

3 ↗

(On Diff #70017)

enum A { A = 1,
         B = 2,
         C = 4,
         D = 8,
         E = 16,
         F = 32,
         G = 63
};

should be:

enum A {
  A = 1,
  B = 2,
  C = 4,
  D = 8,
  E = 16,
  F = 32,
  G = 63
};

is what I was thinking was incorrect, but perhaps clang-format allows such constructs?

66 ↗

(On Diff #70017)

Do you intend to have the return 0; here?

[szepet]

cast to dyn-cast change in order to fix a bug, changes based on comments

[alexfh]

Close, but still a bunch of comments in the docs and a suggestion to fix a class of false positives.

clang-tidy/misc/EnumMisuseCheck.cpp

210 ↗

(On Diff #70324)

1. llvm/lib/MC/ELFObjectWriter.cpp:903 - the warning looks reasonable.
2. llvm/lib/Target/X86/Disassembler/X86DisassemblerDecoderCommon.h:66 - the warning looks reasonable (ATTR_max doesn't seem to be useful for the bitmask enum).
3. llvm/tools/clang/lib/Basic/IdentifierTable.cpp:95 - two issues here:
   1. the "possibly contains misspelled number(s) " message could be more useful, if it specified which member corresponds to the possibly misspelled number
   2. I suppose, the check considers KEYALL = (0x1fffff & ~KEYNOMS18 & ~KEYNOOPENCL) to be incorrect. I think, it should exclude enum members initialized with a bit arithmetic expressions, since it's rather common to define aliases for a certain combination of flags.
4. llvm/tools/clang/lib/Frontend/Rewrite/RewriteModernObjC.cpp:5083 and friends - the warning looks reasonable, since it's hard to understand the motivation for the BLOCK_FIELD_IS_OBJECT = 3. If it's a combination of flags, it should be written as such, and the check should ignore enum members initialized with a bit arithmetic expression.

docs/clang-tidy/checks/misc-enum-misuse.rst

9 ↗

(On Diff #70324)

Too much indentation here.

38 ↗

(On Diff #70324)

1. Enum should not start with a capital letter, since it's a keyword.
2. Please indent the code block contents by 2 spaces (currently, it's indented by 1).
3. Please clang-format all code samples.

47 ↗

(On Diff #70324)

Commas should be used instead of semicolons.

54 ↗

(On Diff #70324)

Missing semicolon.

In general, please make sure code snippets are valid code. Otherwise, it creates unneeded obstacles in reading the code.

[szepet]

In order to decrease false positive rate, the bitmask specific checker part investigate only the enumconstans which was initilized by a literal. (If this is too strong it can be modified)

Renamed the checker to be more consistent with the checkers used for similar purpose.

Documentation code examples updated.

[alexfh]

Thank you for the updates!

Please re-run the check on LLVM to see what has changed.

clang-tidy/misc/SuspiciousEnumUsageCheck.cpp

53

nit: Add an empty line above.

61

nit: Add an empty line above.

67

nit: Please add braces, since the body doesn't fit on a line.

155

Why?

175

Use early return here.

208

One variable definition at a time, please.

216

Use early return here.

clang-tidy/misc/SuspiciousEnumUsageCheck.h

32–35

These can be just static constants in the .cpp file. Apart from that, const char X[] = ...; is a better way to define string constants, otherwise you would have to go with const char * const X = ...; to make the pointer const as well.

docs/clang-tidy/checks/misc-suspicious-enum-usage.rst

4

This line should be aligned with the line above.

9

Remove two spaces at the start of the line.

11

There's no "Weak" option, it's the "Strict" option set to false / 0.

12

Please use the :option: notation and add the option description (.. option:: ...). See, for example, modernize-use-emplace.rst.

31

This notation is used to make the previous line a heading. Doesn't seem to be the case here. See http://llvm.org/docs/SphinxQuickstartTemplate.html for some examples. Please also try to build your docs to check for obvious issues.

test/clang-tidy/misc-suspicious-enum-usage-strict.cpp

18

Add check lines for notes as well (I don't think you'll be able to use [[@LINE]] for most of them, but you can probably just skip the line.

[szepet]

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. )

So strict mode results in this revision where we investigate only the literals:

results.txt4 KBDownload

[szepet]

clang-tidy/misc/SuspiciousEnumUsageCheck.cpp

155

Because the hasDisjointValueRange function could not decide the values properly. So in case of an empty Enum it would not make sense. Fortunately we know that the empty case should not be reported so used early return on this.

That is why this is needed if we want a deterministic check.

[szepet]

Note message checks added to testfiles.

[szepet]

What is your opinion about the new results? I hope the checker can make it into 4.0.

[alexfh]

LG with one nit. Feel free to ping earlier next time.

clang-tidy/misc/SuspiciousEnumUsageCheck.cpp

171–172

Looks like this is the same as in case 3 below, so you could just move this check out of the branch and remove the duplication below.

[alexfh]

clang-tidy/misc/SuspiciousEnumUsageCheck.cpp

155

BTW, this might make sense to be explained in the comment in the code itself (code review comments are bad means of documenting code).

[szepet]

Minor changes to improve the readability of the code according to comments.

[alexfh]

A few more notes, all fine for a follow up.

clang-tidy/misc/SuspiciousEnumUsageCheck.cpp

203

Looks like you're doing exactly same thing twice for lhs and rhs. Pull this out to a function. Fine for a follow up.

213–214

There's not much value in these variables.

216–220

This code doesn't need the lhs/rhs variable declared above it. Move it up.

221–228

This code can be pulled to a function / method to avoid repeating it twice (starting from the const auto *LhsExpr = Result.Nodes.getNodeAs<Expr>("lhsExpr"); part).

docs/clang-tidy/checks/misc-suspicious-enum-usage.rst

31

This is not done yet.

[aaron.ballman]

clang-tidy/misc/SuspiciousEnumUsageCheck.cpp

32

Please drop the (s) from the diagnostic. The phrase "but some literal are not" is incorrect. Alternatively, you could use the %plural diagnostic modifier (see note_constexpr_baa_value_insufficient_alignment in DiagnosticASTKinds.td for an example usage).

[szepet]

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.)

clang-tidy/misc/CMakeLists.txt

add_clang_library(clangTidyMiscModule
   PointerAndIntegralOperationCheck.cpp
   RedundantExpressionCheck.cpp
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
   StaticAssertCheck.cpp
   StringConstructorCheck.cpp
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp
+  SuspiciousEnumUsageCheck.cpp
   SuspiciousMissingCommaCheck.cpp
   SuspiciousSemicolonCheck.cpp
   SuspiciousStringCompareCheck.cpp
   SwappedArgumentsCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp
   UndelegatedConstructor.cpp
   UniqueptrResetReleaseCheck.cpp
   UnusedAliasDeclsCheck.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "PointerAndIntegralOperationCheck.h"
 #include "RedundantExpressionCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "StaticAssertCheck.h"
 #include "StringConstructorCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
+#include "SuspiciousEnumUsageCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<StaticAssertCheck>(
         "misc-static-assert");
     CheckFactories.registerCheck<StringConstructorCheck>(
         "misc-string-constructor");
     CheckFactories.registerCheck<StringIntegerAssignmentCheck>(
         "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>(
         "misc-suspicious-semicolon");
     CheckFactories.registerCheck<SuspiciousStringCompareCheck>(
         "misc-suspicious-string-compare");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "misc-swapped-arguments");

clang-tidy/misc/SuspiciousEnumUsageCheck.h

+//===--- 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;
+  const char* DifferentEnumErrorMessage;
+  const char* BitmaskErrorMessage;
+  const char* BitmaskVarErrorMessage;
+  const char* BitmaskNoteMessage;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SUSPICIOUS_ENUM_USAGE_H

clang-tidy/misc/SuspiciousEnumUsageCheck.cpp

+//===--- 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 {
+
+/// 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 isNonPowerOf2NorNullLiteral(const EnumConstantDecl *EnumConst) {
+  if (const Expr *InitExpr = EnumConst->getInitExpr()) {
+    llvm::APSInt Val = EnumConst->getInitVal();
+    return Val.getBoolValue() && !Val.isPowerOf2() &&
+           dyn_cast_or_null<IntegerLiteral>(InitExpr->IgnoreImpCasts());
+  }
+  return false;
+}
+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() &&
+           dyn_cast<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 < enumLength(EnumDec) / 2 &&
+         (VR.MaxVal - VR.MinVal != EnumLen - 1) &&
+         !(NonPowOfTwoCounter == 1 && isMaxValAllBitSetLiteral(EnumDec));
+}
+
+SuspiciousEnumUsageCheck::SuspiciousEnumUsageCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context), StrictMode(Options.get("StrictMode", 1)) {
+  DifferentEnumErrorMessage = "enum values are from different enum types";
+
+  BitmaskErrorMessage = "enum type seems like a bitmask (contains mostly "
+                        "power-of-2 literals), but this literal is not a "
+                        "power-of-2";
+  BitmaskNoteMessage = "used here as a bitmask";
+
+  BitmaskVarErrorMessage =
+      "enum type seems like a bitmask (contains mostly "
+      "power-of-2 literals) but some literal(s) are not a power-of-2";
+}
+
+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")))))))
+          .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 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");
+
+    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 = countNonPowOfTwoLiteralNum(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 && 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
+  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 = countNonPowOfTwoLiteralNum(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(), BitmaskVarErrorMessage);
+      diag(LhsExpr->getExprLoc(), BitmaskNoteMessage, DiagnosticIDs::Note);
+    } else if (isNonPowerOf2NorNullLiteral(LhsConstant)) {
+      diag(LhsConstant->getSourceRange().getBegin(), BitmaskErrorMessage);
+      diag(LhsExpr->getExprLoc(), BitmaskNoteMessage, DiagnosticIDs::Note);
+    }
+    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

docs/clang-tidy/checks/list.rst

.. toctree::
    misc-pointer-and-integral-operation
    misc-redundant-expression
    misc-sizeof-container
    misc-sizeof-expression
    misc-static-assert
    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
    misc-swapped-arguments
    misc-throw-by-value-catch-by-reference
    misc-unconventional-assign-operator
    misc-undelegated-constructor
    misc-uniqueptr-reset-release

docs/clang-tidy/checks/misc-suspicious-enum-usage.rst

+.. 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.
+
+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++
+
+  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.

test/clang-tidy/misc-suspicious-enum-usage-strict.cpp

+// 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;
+}

test/clang-tidy/misc-suspicious-enum-usage.cpp

+// 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,
+  // 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]
+  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 
+  if (I | Y)
+    return 1;
+  // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+  unsigned p;
+  p = Q | P;
+}
+
+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;
+}