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