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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

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



================
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:202
+
+  const auto *LhsExpr = Result.Nodes.getNodeAs<Expr>("lhsExpr");
+  const auto *RhsExpr = Result.Nodes.getNodeAs<Expr>("rhsExpr");
----------------
Looks like you're doing exactly same thing twice for lhs and rhs. Pull this out to a function. Fine for a follow up.


================
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:212-213
+      RhsDecExpr ? dyn_cast<EnumConstantDecl>(RhsDecExpr->getDecl()) : nullptr;
+  bool LhsVar = !LhsConstant;
+  bool RhsVar = !RhsConstant;
+
----------------
There's not much value in these variables.


================
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:215-219
+  int NonPowOfTwoCounter = countNonPowOfTwoLiteralNum(EnumDec);
+  ValueRange VR(EnumDec);
+  int EnumLen = enumLength(EnumDec);
+  if (!isPossiblyBitMask(NonPowOfTwoCounter, EnumLen, VR, EnumDec))
+    return;
----------------
This code doesn't need the lhs/rhs variable declared above it. Move it up.


================
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:220-227
+  // 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);
----------------
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).


================
Comment at: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst:31
+
+===============
+
----------------
alexfh wrote:
> 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.
This is not done yet.


https://reviews.llvm.org/D22507