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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits