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

[Aaron Ballman via cfe-commits]

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