[PATCH] D22507: Clang-tidy - Enum misuse check

[Alexander Kornienko via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

Thank you for the updates!

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


================
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:53
@@ +52,3 @@
+}
+static bool isNonPowerOf2NorNullLiteral(const EnumConstantDecl *EnumConst) {
+  if (const Expr *InitExpr = EnumConst->getInitExpr()) {
----------------
nit: Add an empty line above.

================
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:61
@@ +60,3 @@
+}
+static bool isMaxValAllBitSetLiteral(const EnumDecl *EnumDec) {
+  auto EnumConst = std::max_element(
----------------
nit: Add an empty line above.

================
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:67
@@ +66,3 @@
+      });
+  if (const Expr *InitExpr = EnumConst->getInitExpr())
+    return EnumConst->getInitVal().countTrailingOnes() ==
----------------
nit: Please add braces, since the body doesn't fit on a line.

================
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:155
@@ +154,3 @@
+
+    if (EnumDec->enumerator_begin() == EnumDec->enumerator_end() ||
+        OtherEnumDec->enumerator_begin() == OtherEnumDec->enumerator_end())
----------------
Why?

================
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:175
@@ +174,3 @@
+    int NonPowOfTwoCounter = countNonPowOfTwoLiteralNum(EnumDec);
+    if (isPossiblyBitMask(NonPowOfTwoCounter, EnumLen, VR, EnumDec)) {
+      const auto *EnumDecExpr = dyn_cast<DeclRefExpr>(EnumExpr);
----------------
Use early return here.

================
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:208
@@ +207,3 @@
+      RhsDecExpr ? dyn_cast<EnumConstantDecl>(RhsDecExpr->getDecl()) : nullptr;
+  bool LhsVar = !LhsConstant, RhsVar = !RhsConstant;
+
----------------
One variable definition at a time, please.

================
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:216
@@ +215,3 @@
+  int EnumLen = enumLength(EnumDec);
+  if (isPossiblyBitMask(NonPowOfTwoCounter, EnumLen, VR, EnumDec) &&
+      (StrictMode ||
----------------
Use early return here.

================
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.h:32-35
@@ +31,6 @@
+  const bool StrictMode;
+  const char* DifferentEnumErrorMessage;
+  const char* BitmaskErrorMessage;
+  const char* BitmaskVarErrorMessage;
+  const char* BitmaskNoteMessage;
+};
----------------
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.

================
Comment at: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst:4
@@ +3,3 @@
+misc-suspicious-enum-usage
+================
+
----------------
This line should be aligned with the line above.

================
Comment at: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst:9
@@ +8,3 @@
+1. When "ADD" or "bitwise OR" is used between two enum which come from different
+     types and these types value ranges are not disjoint.
+
----------------
Remove two spaces at the start of the line.

================
Comment at: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst:11
@@ +10,3 @@
+
+In the following cases you can choose either "Strict" or "Weak" option.
+In "Strict" mode we check if the used EnumConstantDecl type contains almost
----------------
There's no "Weak" option, it's the "Strict" option set to false / 0. 

================
Comment at: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst:12
@@ +11,3 @@
+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. 
----------------
Please use the `:option:` notation and add the option description (`.. option:: ...`). See, for example, modernize-use-emplace.rst.

================
Comment at: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst:31
@@ +30,3 @@
+
+===============
+
----------------
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.

================
Comment at: test/clang-tidy/misc-suspicious-enum-usage-strict.cpp:18
@@ +17,3 @@
+  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]
+};
----------------
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.


https://reviews.llvm.org/D22507