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

Etienne Bergeron via cfe-commits cfe-commits at lists.llvm.org
Tue Jul 19 10:27:07 PDT 2016


etienneb added a comment.

drive-by, some comments.
Thanks for the check


================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:20
@@ +19,3 @@
+// Return the number of EnumConstantDecls in an EnumDecl.
+static int enumLen(const EnumDecl *EnumDec) {
+  int Counter = 0;
----------------
hokein wrote:
> You can use `std::distance(Enum->enumerator_begin(), Enum->enumerator_end())`.
We tend to keep name meaningful and avoid abbreviation.
enumLen -> enumLength

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:35
@@ +34,3 @@
+  ValueRange(const EnumDecl *EnumDec) {
+
+    llvm::APSInt BeginVal = EnumDec->enumerator_begin()->getInitVal();
----------------
nit: remove empty line

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:55
@@ +54,3 @@
+
+bool hasCommonBit(const llvm::APSInt &Val1, const llvm::APSInt &Val2) {
+  return (Val1 & Val2).getExtValue();
----------------
nit: static

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:81
@@ +80,3 @@
+void EnumMisuseCheck::registerMatchers(MatchFinder *Finder) {
+
+  const auto enumExpr = [](StringRef RefName, StringRef DeclName) {
----------------
nit: remove empty line

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:117
@@ +116,3 @@
+// if there is only one not power-of-2 value in the enum unless it is
+static bool isPossiblyBitField(const int NonPowOfTwoCounter, const int EnumLen,
+                               const ValueRange &VR, const EnumDecl *EnumDec) {
----------------
nit: const int x   --> int x   for all parameters

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:119
@@ +118,3 @@
+                               const ValueRange &VR, const EnumDecl *EnumDec) {
+  return NonPowOfTwoCounter >= 1 && NonPowOfTwoCounter <= 2 && NonPowOfTwoCounter < enumLen(EnumDec) / 2 &&
+         (VR.MaxVal - VR.MinVal != EnumLen - 1) && !(NonPowOfTwoCounter == 1 && isMaxValAllBitSet(EnumDec));
----------------
nit: line too long

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:127
@@ +126,3 @@
+  // 1. case: The two enum values come from different types.
+  if (DiffEnumOp) {
+
----------------
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.

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:128
@@ +127,3 @@
+  if (DiffEnumOp) {
+
+    const auto *EnumDec = Result.Nodes.getNodeAs<EnumDecl>("enumDecl");
----------------
nit: no empty line

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:150
@@ +149,3 @@
+
+  if (EnumExpr) {
+    if (!IsStrict)
----------------
ditto for th "if (var = ...)" pattern 

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:170
@@ +169,3 @@
+                                          "possibly contains misspelled "
+                                          "number(s) ");
+        diag(EnumExpr->getExprLoc(), "Used here as a bitfield.",
----------------
nit: remove the space after (s)

================
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:177
@@ +176,3 @@
+  }
+  // 3. case
+  // | or + operator where both argument comes from the same enum type
----------------
To be consistent with your comments, add newline here.


Repository:
  rL LLVM

https://reviews.llvm.org/D22507





More information about the cfe-commits mailing list