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

[Alexander Kornienko via cfe-commits]

alexfh requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:28
@@ +27,3 @@
+    llvm::APSInt BeginVal = EnumDec->enumerator_begin()->getInitVal();
+    {
+      const auto MinMaxVal = std::minmax_element(
----------------
I don't think the compound statement here is needed.

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:48
@@ +47,3 @@
+  ValueRange Range1(Enum1), Range2(Enum2);
+  return ((Range1.MaxVal < Range2.MinVal) || (Range2.MaxVal < Range1.MinVal));
+}
----------------
Please remove the outermost parentheses, they are superfluous.

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:56
@@ +55,3 @@
+bool isMaxValAllBitSet(const EnumDecl *EnumDec) {
+
+  auto It = std::max_element(
----------------
nit: Please remove the empty line.

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:59
@@ +58,3 @@
+      EnumDec->enumerator_begin(), EnumDec->enumerator_end(),
+      [](const EnumConstantDecl *It1, const EnumConstantDecl *It2) {
+        return It1->getInitVal() < It2->getInitVal();
----------------
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`.

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:128
@@ +127,3 @@
+void EnumMisuseCheck::check(const MatchFinder::MatchResult &Result) {
+
+  // 1. case: The two enum values come from different types.
----------------
Please remove the empty line.

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:146
@@ +145,3 @@
+
+  // 2. case:
+  //   a, Investigating the right hand side of += or |= operator.
----------------
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`).

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:164
@@ +163,3 @@
+
+      if (EnumConstDecl && !(EnumConstDecl->getInitVal().isPowerOf2()))
+        diag(EnumExpr->getExprLoc(), "enum value is not power-of-2 unlike "
----------------
No parentheses needed after `!`.

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:164
@@ +163,3 @@
+
+      if (EnumConstDecl && !(EnumConstDecl->getInitVal().isPowerOf2()))
+        diag(EnumExpr->getExprLoc(), "enum value is not power-of-2 unlike "
----------------
alexfh wrote:
> No parentheses needed after `!`.
Braces should be used consistently in each if-else chain. Please add braces around the first `if` body.

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:172
@@ +171,3 @@
+                                          "number(s)");
+        diag(EnumExpr->getExprLoc(), "Used here as a bitfield.",
+             DiagnosticIDs::Note);
----------------
No capitalization and trailing period needed.

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:224
@@ +223,3 @@
+           DiagnosticIDs::Note);
+    } else if (!(RhsConstant->getInitVal()).isPowerOf2())
+      diag(RhsExpr->getExprLoc(), "right hand side value is not power-of-2 "
----------------
Inner parentheses are not needed.

================
Comment at: clang-tidy/misc/EnumMisuseCheck.h:25
@@ +24,3 @@
+private:
+  const bool IsStrict;
+
----------------
I'd move the private section to the bottom of the class definition.


https://reviews.llvm.org/D22507