aaron.ballman added inline comments. ================ Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:40 @@ +39,3 @@ + +// Return the number of EnumConstantDecls in an EnumDecl. +static int enumLength(const EnumDecl *EnumDec) { ---------------- Doxygen comment here as well.
================ Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:75 @@ +74,3 @@ +// We check if there is at most 2 not power-of-2 value in the enum type and +// the +// it contains enough element to make sure it could be a biftield, but we ---------------- The wrapping for this comment is a bit strange, also should be using doxygen-style comments. Also, the grammar is a bit off for the comment. I would recommend: ``` Check if there are two or more enumerators that are not a power of 2 in the enum type, and that the enumeration contains enough elements to reasonably act as a bitmask. Exclude the case where the last enumerator is the sum of the lesser values or when it could contain consecutive values. ``` Also, I would call this `isPossiblyBitMask` instead of using "bit field" because a bit-field is a syntactic construct that is unrelated. Bitmask types are covered in the C++ standard under [bitmask.types] and are slightly different, but more closely related to what this check is looking for. ================ Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:209 @@ +208,3 @@ + if (LhsVar) { + diag(EnumDec->getInnerLocStart(), "enum type seems like a bitfield but " + "possibly contains misspelled " ---------------- I think this diagnostic text is a bit confusing. The enum type shouldn't seem like a bit-field (but more like a bitmask), and I'm not certain what a misspelled number would be. I think the diagnostic is effectively saying that we guess this might be a bitmask, but some of the enumerator values don't make sense for that guess, and so this may be suspicious code -- but I really worry about the false positive rate for such a diagnostic. Have you run this check over a large body of work (like LLVM, Firefox, Chrome, etc)? If so, how do the diagnostics look? Perhaps a different way to word the diagnostic is: "enum type used as a bitmask with an enumerator value that is not a power of 2" and place the diagnostic on the enumerator(s) that cause the problem rather than the enumeration as a whole. ================ Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:212 @@ +211,3 @@ + "number(s)"); + diag(LhsExpr->getExprLoc(), "Used here as a bitfield.", + DiagnosticIDs::Note); ---------------- s/bitmask/bitfield ================ Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:214 @@ +213,3 @@ + DiagnosticIDs::Note); + } else if (!(LhsConstant->getInitVal()).isPowerOf2()) + diag(LhsExpr->getExprLoc(), "left hand side value is not power-of-2 " ---------------- Are these spurious parens around LhsConstant->getInitVal()? ================ Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:219 @@ +218,3 @@ + if (RhsVar) { + diag(EnumDec->getInnerLocStart(), "enum type seems like a bitfield but " + "possibly contains misspelled " ---------------- Same comments here as above; if you can remove the duplicate code, that would be great, too. ================ Comment at: clang-tidy/misc/EnumMisuseCheck.h:24 @@ +23,3 @@ +class EnumMisuseCheck : public ClangTidyCheck { +private: + const bool IsStrict; ---------------- Can remove the private access specifier -- it's already private. ================ Comment at: test/clang-tidy/misc-enum-misuse-weak.cpp:1 @@ +1,2 @@ +// RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions: [{key: misc-enum-misuse.IsStrict, value: 1}]}" -- + ---------------- Please run clang-format over this file. ================ Comment at: test/clang-tidy/misc-enum-misuse.cpp:1 @@ +1,2 @@ +// RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions: [{key: misc-enum-misuse.IsStrict, value: 0}]}" -- + ---------------- Please run clang-format over this file. https://reviews.llvm.org/D22507 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits