[PATCH] D22507: Clang-tidy - Enum misuse check
This revision was automatically updated to reflect the committed changes.
Closed by commit rL290600: [clang-tidy] Add enum misuse check. (authored by
xazax).
Changed prior to commit:
https://reviews.llvm.org/D22507?vs=82488=82533#toc
Repository:
rL LLVM
https://reviews.llvm.org/D22507
Files:
clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
clang-tools-extra/trunk/clang-tidy/misc/SuspiciousEnumUsageCheck.cpp
clang-tools-extra/trunk/clang-tidy/misc/SuspiciousEnumUsageCheck.h
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-enum-usage.cpp
Index: clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-enum-usage.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-enum-usage.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-enum-usage.cpp
@@ -0,0 +1,90 @@
+// RUN: %check_clang_tidy %s misc-suspicious-enum-usage %t -- -config="{CheckOptions: [{key: misc-suspicious-enum-usage.StrictMode, value: 0}]}" --
+
+enum Empty {
+};
+
+enum A {
+ A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X {
+ X = 8,
+ Y = 16,
+ Z = 4
+};
+
+enum {
+ P = 2,
+ Q = 3,
+ R = 4,
+ S = 8,
+ T = 16
+};
+
+enum {
+ H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days {
+ Monday,
+ Tuesday,
+ Wednesday,
+ Thursday,
+ Friday,
+ Saturday,
+ Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ Empty EmptyVal;
+ int emptytest = EmptyVal | B;
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+}
+
+int dont_trigger() {
+ unsigned p;
+ p = Q | P;
+
+ if (A + G == E)
+return 1;
+ else if ((Q | R) == T)
+return 1;
+ else
+int k = T | Q;
+
+ Empty EmptyVal;
+ int emptytest = EmptyVal | B;
+
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
@@ -0,0 +1,98 @@
+// RUN: %check_clang_tidy %s misc-suspicious-enum-usage %t -- -config="{CheckOptions: [{key: misc-suspicious-enum-usage.StrictMode, value: 1}]}" --
+
+enum A {
+ A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: enum type seems like a bitmask (contains mostly power-of-2 literals) but a literal is not power-of-2
+// CHECK-MESSAGES: :76:7: note: used here as a bitmask
+enum X {
+ X = 8,
+ Y = 16,
+ Z = 4,
+ ZZ = 3
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask (contains mostly power-of-2 literals), but this literal is not a power-of-2 [misc-suspicious-enum-usage]
+// CHECK-MESSAGES: :70:13: note: used here as a bitmask
+};
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: enum type seems like a bitmask (contains mostly power-of-2 literals) but some literals are not power-of-2
+// CHECK-MESSAGES: :73:8: note: used here as a bitmask
+enum PP {
+ P = 2,
+ Q = 3,
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask (contains mostly power-of-2 literals), but this literal is not a power-of-2
+ // CHECK-MESSAGES: :65:11: note: used here as a bitmask
+ R = 4,
+ S = 8,
+ T = 16,
+ U = 31
+};
+
+enum {
+ H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days {
+ Monday,
+ Tuesday,
+ Wednesday,
+ Thursday,
+ Friday,
+ Saturday,
+ Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+ if (P + Q == R)
+return 1;
+ else if ((Q | R) == T)
+return 1;
+ else
+int k = ZZ | Z;
+ unsigned p = R;
+ PP pp = Q;
+ p |= pp;
+
+ enum X x = Z;
+ p = x | ZZ;
+ return 0;
+}
+
+int dont_trigger() {
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+
+ unsigned bitflag;
+ enum A aa = B;
+ bitflag = aa | C;
+
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday
[PATCH] D22507: Clang-tidy - Enum misuse check
szepet updated this revision to Diff 82488.
szepet marked 6 inline comments as done.
szepet added a comment.
The requested changes have been made.
Some more refactor on the Case2 since it is the same as the LHS/RHS case. Moved
more common statements out of the branch (Case2-3) for better readabilty. (And
less code duplication.)
https://reviews.llvm.org/D22507
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/SuspiciousEnumUsageCheck.cpp
clang-tidy/misc/SuspiciousEnumUsageCheck.h
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
test/clang-tidy/misc-suspicious-enum-usage.cpp
Index: test/clang-tidy/misc-suspicious-enum-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-suspicious-enum-usage.cpp
@@ -0,0 +1,90 @@
+// RUN: %check_clang_tidy %s misc-suspicious-enum-usage %t -- -config="{CheckOptions: [{key: misc-suspicious-enum-usage.StrictMode, value: 0}]}" --
+
+enum Empty {
+};
+
+enum A {
+ A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X {
+ X = 8,
+ Y = 16,
+ Z = 4
+};
+
+enum {
+ P = 2,
+ Q = 3,
+ R = 4,
+ S = 8,
+ T = 16
+};
+
+enum {
+ H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days {
+ Monday,
+ Tuesday,
+ Wednesday,
+ Thursday,
+ Friday,
+ Saturday,
+ Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ Empty EmptyVal;
+ int emptytest = EmptyVal | B;
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+}
+
+int dont_trigger() {
+ unsigned p;
+ p = Q | P;
+
+ if (A + G == E)
+return 1;
+ else if ((Q | R) == T)
+return 1;
+ else
+int k = T | Q;
+
+ Empty EmptyVal;
+ int emptytest = EmptyVal | B;
+
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
@@ -0,0 +1,98 @@
+// RUN: %check_clang_tidy %s misc-suspicious-enum-usage %t -- -config="{CheckOptions: [{key: misc-suspicious-enum-usage.StrictMode, value: 1}]}" --
+
+enum A {
+ A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: enum type seems like a bitmask (contains mostly power-of-2 literals) but a literal is not power-of-2
+// CHECK-MESSAGES: :76:7: note: used here as a bitmask
+enum X {
+ X = 8,
+ Y = 16,
+ Z = 4,
+ ZZ = 3
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask (contains mostly power-of-2 literals), but this literal is not a power-of-2 [misc-suspicious-enum-usage]
+// CHECK-MESSAGES: :70:13: note: used here as a bitmask
+};
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: enum type seems like a bitmask (contains mostly power-of-2 literals) but some literals are not power-of-2
+// CHECK-MESSAGES: :73:8: note: used here as a bitmask
+enum PP {
+ P = 2,
+ Q = 3,
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask (contains mostly power-of-2 literals), but this literal is not a power-of-2
+ // CHECK-MESSAGES: :65:11: note: used here as a bitmask
+ R = 4,
+ S = 8,
+ T = 16,
+ U = 31
+};
+
+enum {
+ H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days {
+ Monday,
+ Tuesday,
+ Wednesday,
+ Thursday,
+ Friday,
+ Saturday,
+ Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+ if (P + Q == R)
+return 1;
+ else if ((Q | R) == T)
+return 1;
+ else
+int k = ZZ | Z;
+ unsigned p = R;
+ PP pp = Q;
+ p |= pp;
+
+ enum X x = Z;
+ p = x | ZZ;
+ return 0;
+}
+
+int dont_trigger() {
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+
+ unsigned bitflag;
+ enum A aa = B;
+ bitflag = aa | C;
+
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
@@ -0,0
[PATCH] D22507: Clang-tidy - Enum misuse check
aaron.ballman added inline comments. Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:31 +"enum type seems like a bitmask (contains mostly " +"power-of-2 literals) but some literal(s) are not a power-of-2"; + Please drop the `(s)` from the diagnostic. The phrase "but some literal are not" is incorrect. Alternatively, you could use the `%plural` diagnostic modifier (see `note_constexpr_baa_value_insufficient_alignment` in DiagnosticASTKinds.td for an example usage). https://reviews.llvm.org/D22507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22507: Clang-tidy - Enum misuse check
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("lhsExpr");
+ const auto *RhsExpr = Result.Nodes.getNodeAs("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(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("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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22507: Clang-tidy - Enum misuse check
szepet updated this revision to Diff 81848.
szepet added a comment.
Herald added a subscriber: JDevlieghere.
Minor changes to improve the readability of the code according to comments.
https://reviews.llvm.org/D22507
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/SuspiciousEnumUsageCheck.cpp
clang-tidy/misc/SuspiciousEnumUsageCheck.h
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
test/clang-tidy/misc-suspicious-enum-usage.cpp
Index: test/clang-tidy/misc-suspicious-enum-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-suspicious-enum-usage.cpp
@@ -0,0 +1,90 @@
+// RUN: %check_clang_tidy %s misc-suspicious-enum-usage %t -- -config="{CheckOptions: [{key: misc-suspicious-enum-usage.StrictMode, value: 0}]}" --
+
+enum Empty {
+};
+
+enum A {
+ A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X {
+ X = 8,
+ Y = 16,
+ Z = 4
+};
+
+enum {
+ P = 2,
+ Q = 3,
+ R = 4,
+ S = 8,
+ T = 16
+};
+
+enum {
+ H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days {
+ Monday,
+ Tuesday,
+ Wednesday,
+ Thursday,
+ Friday,
+ Saturday,
+ Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ Empty EmptyVal;
+ int emptytest = EmptyVal | B;
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+}
+
+int dont_trigger() {
+ unsigned p;
+ p = Q | P;
+
+ if (A + G == E)
+return 1;
+ else if ((Q | R) == T)
+return 1;
+ else
+int k = T | Q;
+
+ Empty EmptyVal;
+ int emptytest = EmptyVal | B;
+
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
@@ -0,0 +1,94 @@
+// RUN: %check_clang_tidy %s misc-suspicious-enum-usage %t -- -config="{CheckOptions: [{key: misc-suspicious-enum-usage.StrictMode, value: 1}]}" --
+
+enum A {
+ A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X {
+ X = 8,
+ Y = 16,
+ Z = 4,
+ ZZ = 3
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask (contains mostly power-of-2 literals), but this literal is not a power-of-2 [misc-suspicious-enum-usage]
+// CHECK-MESSAGES: :68:13: note: used here as a bitmask
+};
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: enum type seems like a bitmask (contains mostly power-of-2 literals) but some literal(s) are not a power-of-2
+ // CHECK-MESSAGES: :71:8: note: used here as a bitmask
+enum PP {
+ P = 2,
+ Q = 3,
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask (contains mostly power-of-2 literals), but this literal is not a power-of-2
+ // CHECK-MESSAGES: :63:7: note: used here as a bitmask
+ R = 4,
+ S = 8,
+ T = 16,
+ U = 31
+};
+
+enum {
+ H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days {
+ Monday,
+ Tuesday,
+ Wednesday,
+ Thursday,
+ Friday,
+ Saturday,
+ Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+ if (P + Q == R)
+return 1;
+ else if ((Q | R) == T)
+return 1;
+ else
+int k = ZZ | Z;
+ unsigned p = R;
+ PP pp = Q;
+ p |= pp;
+ p = A | G;
+ return 0;
+}
+
+int dont_trigger() {
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+
+ unsigned bitflag;
+ enum A aa = B;
+ bitflag = aa | C;
+
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
@@ -0,0 +1,80 @@
+.. title:: clang-tidy - misc-suspicious-enum-usage
+
+misc-suspicious-enum-usage
+==
+
+The checker detects various cases when an enum is probably misused (as a bitmask
+).
+
+1. When "ADD" or "bitwise OR" is used between two enum which come from different
+ types and these types value ranges are not disjoint.
+
+The following
[PATCH] D22507: Clang-tidy - Enum misuse check
alexfh added inline comments. Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:155 + +if (EnumDec->enumerator_begin() == EnumDec->enumerator_end() || +OtherEnumDec->enumerator_begin() == OtherEnumDec->enumerator_end()) szepet wrote: > alexfh wrote: > > Why? > Because the hasDisjointValueRange function could not decide the values > properly. So in case of an empty Enum it would not make sense. Fortunately we > know that the empty case should not be reported so used early return on this. > > That is why this is needed if we want a deterministic check. BTW, this might make sense to be explained in the comment in the code itself (code review comments are bad means of documenting code). https://reviews.llvm.org/D22507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22507: Clang-tidy - Enum misuse check
alexfh added a comment.
LG with one nit. Feel free to ping earlier next time.
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:170-171
+ if (const auto *EnumExpr = Result.Nodes.getNodeAs("enumExpr")) {
+if (!StrictMode)
+ return;
+const auto *EnumDec = Result.Nodes.getNodeAs("enumDecl");
Looks like this is the same as in case 3 below, so you could just move this
check out of the branch and remove the duplication below.
https://reviews.llvm.org/D22507
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22507: Clang-tidy - Enum misuse check
szepet added a comment. What is your opinion about the new results? I hope the checker can make it into 4.0. https://reviews.llvm.org/D22507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22507: Clang-tidy - Enum misuse check
szepet updated this revision to Diff 73439.
szepet marked an inline comment as done.
szepet added a comment.
Herald added a subscriber: modocache.
Note message checks added to testfiles.
https://reviews.llvm.org/D22507
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/SuspiciousEnumUsageCheck.cpp
clang-tidy/misc/SuspiciousEnumUsageCheck.h
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
test/clang-tidy/misc-suspicious-enum-usage.cpp
Index: test/clang-tidy/misc-suspicious-enum-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-suspicious-enum-usage.cpp
@@ -0,0 +1,90 @@
+// RUN: %check_clang_tidy %s misc-suspicious-enum-usage %t -- -config="{CheckOptions: [{key: misc-suspicious-enum-usage.StrictMode, value: 0}]}" --
+
+enum Empty {
+};
+
+enum A {
+ A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X {
+ X = 8,
+ Y = 16,
+ Z = 4
+};
+
+enum {
+ P = 2,
+ Q = 3,
+ R = 4,
+ S = 8,
+ T = 16
+};
+
+enum {
+ H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days {
+ Monday,
+ Tuesday,
+ Wednesday,
+ Thursday,
+ Friday,
+ Saturday,
+ Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ Empty EmptyVal;
+ int emptytest = EmptyVal | B;
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+}
+
+int dont_trigger() {
+ unsigned p;
+ p = Q | P;
+
+ if (A + G == E)
+return 1;
+ else if ((Q | R) == T)
+return 1;
+ else
+int k = T | Q;
+
+ Empty EmptyVal;
+ int emptytest = EmptyVal | B;
+
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
@@ -0,0 +1,94 @@
+// RUN: %check_clang_tidy %s misc-suspicious-enum-usage %t -- -config="{CheckOptions: [{key: misc-suspicious-enum-usage.StrictMode, value: 1}]}" --
+
+enum A {
+ A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X {
+ X = 8,
+ Y = 16,
+ Z = 4,
+ ZZ = 3
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask (contains mostly power-of-2 literals), but this literal is not a power-of-2 [misc-suspicious-enum-usage]
+// CHECK-MESSAGES: :68:13: note: used here as a bitmask
+};
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: enum type seems like a bitmask (contains mostly power-of-2 literals) but some literal(s) are not a power-of-2
+ // CHECK-MESSAGES: :71:8: note: used here as a bitmask
+enum PP {
+ P = 2,
+ Q = 3,
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask (contains mostly power-of-2 literals), but this literal is not a power-of-2
+ // CHECK-MESSAGES: :63:7: note: used here as a bitmask
+ R = 4,
+ S = 8,
+ T = 16,
+ U = 31
+};
+
+enum {
+ H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days {
+ Monday,
+ Tuesday,
+ Wednesday,
+ Thursday,
+ Friday,
+ Saturday,
+ Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+ if (P + Q == R)
+return 1;
+ else if ((Q | R) == T)
+return 1;
+ else
+int k = ZZ | Z;
+ unsigned p = R;
+ PP pp = Q;
+ p |= pp;
+ p = A | G;
+ return 0;
+}
+
+int dont_trigger() {
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+
+ unsigned bitflag;
+ enum A aa = B;
+ bitflag = aa | C;
+
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
@@ -0,0 +1,80 @@
+.. title:: clang-tidy - misc-suspicious-enum-usage
+
+misc-suspicious-enum-usage
+==
+
+The checker detects various cases when an enum is probably misused (as a bitmask
+).
+
+1. When "ADD" or "bitwise OR" is used between two enum which come from different
+ types and these types value ranges are not disjoint.
+
+The following
[PATCH] D22507: Clang-tidy - Enum misuse check
szepet added inline comments. > alexfh wrote in SuspiciousEnumUsageCheck.cpp:155 > Why? Because the hasDisjointValueRange function could not decide the values properly. So in case of an empty Enum it would not make sense. Fortunately we know that the empty case should not be reported so used early return on this. That is why this is needed if we want a deterministic check. https://reviews.llvm.org/D22507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22507: Clang-tidy - Enum misuse check
szepet updated this revision to Diff 73157.
szepet marked 13 inline comments as done.
szepet added a comment.
Updates based on comments (the testfile note comments will be added in the next
commit)
Some changes in the algorithm/design:
In non-strict mode the checker will only investigate the operations between
different enum types.
In strict mode we check the suspicious bitmasks too.
(In the previous comments we always talked about only the strict mode
heuristics and looked on the strict results. )
https://reviews.llvm.org/D22507
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/SuspiciousEnumUsageCheck.cpp
clang-tidy/misc/SuspiciousEnumUsageCheck.h
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
test/clang-tidy/misc-suspicious-enum-usage.cpp
Index: test/clang-tidy/misc-suspicious-enum-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-suspicious-enum-usage.cpp
@@ -0,0 +1,90 @@
+// RUN: %check_clang_tidy %s misc-suspicious-enum-usage %t -- -config="{CheckOptions: [{key: misc-suspicious-enum-usage.StrictMode, value: 0}]}" --
+
+enum Empty {
+};
+
+enum A {
+ A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X {
+ X = 8,
+ Y = 16,
+ Z = 4
+};
+
+enum {
+ P = 2,
+ Q = 3,
+ R = 4,
+ S = 8,
+ T = 16
+};
+
+enum {
+ H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days {
+ Monday,
+ Tuesday,
+ Wednesday,
+ Thursday,
+ Friday,
+ Saturday,
+ Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ Empty EmptyVal;
+ int emptytest = EmptyVal | B;
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+}
+
+int dont_trigger() {
+ unsigned p;
+ p = Q | P;
+
+ if (A + G == E)
+return 1;
+ else if ((Q | R) == T)
+return 1;
+ else
+int k = T | Q;
+
+ Empty EmptyVal;
+ int emptytest = EmptyVal | B;
+
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
@@ -0,0 +1,92 @@
+// RUN: %check_clang_tidy %s misc-suspicious-enum-usage %t -- -config="{CheckOptions: [{key: misc-suspicious-enum-usage.StrictMode, value: 1}]}" --
+
+enum A {
+ A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X {
+ X = 8,
+ Y = 16,
+ Z = 4,
+ ZZ = 3
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask (contains mostly power-of-2 literals), but this literal is not a power-of-2 [misc-suspicious-enum-usage]
+};
+// CHECK-MESSAGES: :[[@LINE+1]]:1: warning: enum type seems like a bitmask (contains mostly power-of-2 literals) but some literal(s) are not a power-of-2
+enum PP {
+ P = 2,
+ Q = 3,
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask (contains mostly power-of-2 literals), but this literal is not a power-of-2
+ R = 4,
+ S = 8,
+ T = 16,
+ U = 31
+};
+
+enum {
+ H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days {
+ Monday,
+ Tuesday,
+ Wednesday,
+ Thursday,
+ Friday,
+ Saturday,
+ Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+ if (P + Q == R)
+return 1;
+ else if ((Q | R) == T)
+return 1;
+ else
+int k = ZZ | Z;
+ unsigned p = R;
+ PP pp = Q;
+ p |= pp;
+ // [LINE-1] triggers the LINE:17 warning
+ p = A | G;
+ return 0;
+}
+
+int dont_trigger() {
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+
+ unsigned bitflag;
+ enum A aa = B;
+ bitflag = aa | C;
+
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
@@ -0,0 +1,80 @@
+.. title:: clang-tidy - misc-suspicious-enum-usage
+
+misc-suspicious-enum-usage
+==
+
+The checker detects various cases when an enum is
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.
Thank you for the updates!
Please re-run the check on LLVM to see what has changed.
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:53
@@ +52,3 @@
+}
+static bool isNonPowerOf2NorNullLiteral(const EnumConstantDecl *EnumConst) {
+ if (const Expr *InitExpr = EnumConst->getInitExpr()) {
nit: Add an empty line above.
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:61
@@ +60,3 @@
+}
+static bool isMaxValAllBitSetLiteral(const EnumDecl *EnumDec) {
+ auto EnumConst = std::max_element(
nit: Add an empty line above.
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:67
@@ +66,3 @@
+ });
+ if (const Expr *InitExpr = EnumConst->getInitExpr())
+return EnumConst->getInitVal().countTrailingOnes() ==
nit: Please add braces, since the body doesn't fit on a line.
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:155
@@ +154,3 @@
+
+if (EnumDec->enumerator_begin() == EnumDec->enumerator_end() ||
+OtherEnumDec->enumerator_begin() == OtherEnumDec->enumerator_end())
Why?
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:175
@@ +174,3 @@
+int NonPowOfTwoCounter = countNonPowOfTwoLiteralNum(EnumDec);
+if (isPossiblyBitMask(NonPowOfTwoCounter, EnumLen, VR, EnumDec)) {
+ const auto *EnumDecExpr = dyn_cast(EnumExpr);
Use early return here.
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:208
@@ +207,3 @@
+ RhsDecExpr ? dyn_cast(RhsDecExpr->getDecl()) : nullptr;
+ bool LhsVar = !LhsConstant, RhsVar = !RhsConstant;
+
One variable definition at a time, please.
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:216
@@ +215,3 @@
+ int EnumLen = enumLength(EnumDec);
+ if (isPossiblyBitMask(NonPowOfTwoCounter, EnumLen, VR, EnumDec) &&
+ (StrictMode ||
Use early return here.
Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.h:32-35
@@ +31,6 @@
+ const bool StrictMode;
+ const char* DifferentEnumErrorMessage;
+ const char* BitmaskErrorMessage;
+ const char* BitmaskVarErrorMessage;
+ const char* BitmaskNoteMessage;
+};
These can be just static constants in the .cpp file. Apart from that, `const
char X[] = ...;` is a better way to define string constants, otherwise you
would have to go with `const char * const X = ...;` to make the pointer const
as well.
Comment at: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst:4
@@ +3,3 @@
+misc-suspicious-enum-usage
+
+
This line should be aligned with the line above.
Comment at: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst:9
@@ +8,3 @@
+1. When "ADD" or "bitwise OR" is used between two enum which come from
different
+ types and these types value ranges are not disjoint.
+
Remove two spaces at the start of the line.
Comment at: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst:11
@@ +10,3 @@
+
+In the following cases you can choose either "Strict" or "Weak" option.
+In "Strict" mode we check if the used EnumConstantDecl type contains almost
There's no "Weak" option, it's the "Strict" option set to false / 0.
Comment at: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst:12
@@ +11,3 @@
+In the following cases you can choose either "Strict" or "Weak" option.
+In "Strict" mode we check if the used EnumConstantDecl type contains almost
+only pow-of-2 numbers.
Please use the `:option:` notation and add the option description (`.. option::
...`). See, for example, modernize-use-emplace.rst.
Comment at: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst:31
@@ +30,3 @@
+
+===
+
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.
Comment at: test/clang-tidy/misc-suspicious-enum-usage-strict.cpp:18
@@ +17,3 @@
+ ZZ = 3
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask
(contains mostly power-of-2 literals), but this literal is not a power-of-2
[misc-suspicious-enum-usage]
+};
Add check lines for notes as well (I don't think you'll be able to use
[[@LINE]] for most of them, but you can probably just skip the line.
https://reviews.llvm.org/D22507
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
szepet updated the summary for this revision.
szepet updated this revision to Diff 71925.
szepet marked 7 inline comments as done.
szepet added a comment.
Herald added subscribers: mgorny, beanz.
In order to decrease false positive rate, the bitmask specific checker part
investigate only the enumconstans which was initilized by a literal. (If this
is too strong it can be modified)
Renamed the checker to be more consistent with the checkers used for similar
purpose.
Documentation code examples updated.
https://reviews.llvm.org/D22507
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/SuspiciousEnumUsageCheck.cpp
clang-tidy/misc/SuspiciousEnumUsageCheck.h
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
test/clang-tidy/misc-suspicious-enum-usage.cpp
Index: test/clang-tidy/misc-suspicious-enum-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-suspicious-enum-usage.cpp
@@ -0,0 +1,85 @@
+// RUN: %check_clang_tidy %s misc-suspicious-enum-usage %t -- -config="{CheckOptions: [{key: misc-suspicious-enum-usage.StrictMode, value: 0}]}" --
+
+enum Empty {
+};
+
+enum A {
+ A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X {
+ X = 8,
+ Y = 16,
+ Z = 4
+};
+
+enum {
+ P = 2,
+ Q = 3,
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask (contains mostly power-of-2 literals), but this literal is not a power-of-2 [misc-suspicious-enum-usage]
+ R = 4,
+ S = 8,
+ T = 16
+};
+
+enum {
+ H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days {
+ Monday,
+ Tuesday,
+ Wednesday,
+ Thursday,
+ Friday,
+ Saturday,
+ Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+ unsigned p;
+ p = Q | P;
+}
+
+int dont_trigger() {
+ if (A + G == E)
+return 1;
+ else if ((Q | R) == T)
+return 1;
+ else
+int k = T | Q;
+ Empty EmptyVal;
+ int emptytest = EmptyVal | B;
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-suspicious-enum-usage-strict.cpp
@@ -0,0 +1,92 @@
+// RUN: %check_clang_tidy %s misc-suspicious-enum-usage %t -- -config="{CheckOptions: [{key: misc-suspicious-enum-usage.StrictMode, value: 1}]}" --
+
+enum A {
+ A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X {
+ X = 8,
+ Y = 16,
+ Z = 4,
+ ZZ = 3
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask (contains mostly power-of-2 literals), but this literal is not a power-of-2 [misc-suspicious-enum-usage]
+};
+// CHECK-MESSAGES: :[[@LINE+1]]:1: warning: enum type seems like a bitmask (contains mostly power-of-2 literals) but some literal(s) are not a power-of-2
+enum PP {
+ P = 2,
+ Q = 3,
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: enum type seems like a bitmask (contains mostly power-of-2 literals), but this literal is not a power-of-2
+ R = 4,
+ S = 8,
+ T = 16,
+ U = 31
+};
+
+enum {
+ H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days {
+ Monday,
+ Tuesday,
+ Wednesday,
+ Thursday,
+ Friday,
+ Saturday,
+ Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+ if (P + Q == R)
+return 1;
+ else if ((Q | R) == T)
+return 1;
+ else
+int k = ZZ | Z;
+ unsigned p = R;
+ PP pp = Q;
+ p |= pp;
+ // [LINE-1] triggers the LINE:17 warning
+ p = A | G;
+ return 0;
+}
+
+int dont_trigger() {
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+
+ unsigned bitflag;
+ enum A aa = B;
+ bitflag = aa | C;
+
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-suspicious-enum-usage.rst
@@ -0,0 +1,71 @@
+.. title:: clang-tidy -
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.
Close, but still a bunch of comments in the docs and a suggestion to fix a
class of false positives.
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:210
@@ +209,3 @@
+ DiagnosticIDs::Note);
+} else if (!LhsConstant->getInitVal().isPowerOf2())
+ diag(LhsExpr->getExprLoc(), "left hand side value is not power-of-2 "
# llvm/lib/MC/ELFObjectWriter.cpp:903 - the warning looks reasonable.
# llvm/lib/Target/X86/Disassembler/X86DisassemblerDecoderCommon.h:66 - the
warning looks reasonable (ATTR_max doesn't seem to be useful for the bitmask
enum).
# llvm/tools/clang/lib/Basic/IdentifierTable.cpp:95 - two issues here:
1. the "possibly contains misspelled number(s) " message could be more
useful, if it specified which member corresponds to the possibly misspelled
number
2. I suppose, the check considers `KEYALL = (0x1f & ~KEYNOMS18 &
~KEYNOOPENCL)` to be incorrect. I think, it should exclude enum members
initialized with a bit arithmetic expressions, since it's rather common to
define aliases for a certain combination of flags.
# llvm/tools/clang/lib/Frontend/Rewrite/RewriteModernObjC.cpp:5083 and friends
- the warning looks reasonable, since it's hard to understand the motivation
for the `BLOCK_FIELD_IS_OBJECT = 3`. If it's a combination of flags, it
should be written as such, and the check should ignore enum members initialized
with a bit arithmetic expression.
Comment at: docs/clang-tidy/checks/misc-enum-misuse.rst:9
@@ +8,3 @@
+1. When "ADD" or "bitwise OR" is used between two enum which come from
different
+ types and these types value ranges are not disjoint.
+
Too much indentation here.
Comment at: docs/clang-tidy/checks/misc-enum-misuse.rst:38
@@ +37,3 @@
+ // Case 1 (strict and weak mode):
+ Enum {A, B, C};
+ Enum {D, E, F = 5};
1. `Enum` should not start with a capital letter, since it's a keyword.
2. Please indent the code block contents by 2 spaces (currently, it's indented
by 1).
3. Please clang-format all code samples.
Comment at: docs/clang-tidy/checks/misc-enum-misuse.rst:47
@@ +46,3 @@
+ // Case 2 (only in strict mode):
+ Enum bitmask { A = 0;
+B = 1;
Commas should be used instead of semicolons.
Comment at: docs/clang-tidy/checks/misc-enum-misuse.rst:54
@@ +53,3 @@
+G = 31; // OK, real bitmask.
+ }
+
Missing semicolon.
In general, please make sure code snippets are valid code. Otherwise, it
creates unneeded obstacles in reading the code.
https://reviews.llvm.org/D22507
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
szepet updated this revision to Diff 70324.
szepet marked 4 inline comments as done.
szepet added a comment.
cast to dyn-cast change in order to fix a bug, changes based on comments
https://reviews.llvm.org/D22507
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/EnumMisuseCheck.cpp
clang-tidy/misc/EnumMisuseCheck.h
clang-tidy/misc/MiscTidyModule.cpp
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-enum-misuse.rst
test/clang-tidy/misc-enum-misuse-strict.cpp
test/clang-tidy/misc-enum-misuse.cpp
Index: test/clang-tidy/misc-enum-misuse.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-enum-misuse.cpp
@@ -0,0 +1,85 @@
+// RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions: [{key: misc-enum-misuse.StrictMode, value: 0}]}" --
+
+enum Empty {
+};
+
+enum A {
+ A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X {
+ X = 8,
+ Y = 16,
+ Z = 4
+};
+
+enum {
+ P = 2,
+ Q = 3,
+ R = 4,
+ S = 8,
+ T = 16
+};
+
+enum {
+ H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days {
+ Monday,
+ Tuesday,
+ Wednesday,
+ Thursday,
+ Friday,
+ Saturday,
+ Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types [misc-enum-misuse]
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+ unsigned p;
+ p = Q | P;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: left hand side value is not power-of-2 unlike most other values in the enum type
+}
+
+int dont_trigger() {
+ if (A + G == E)
+return 1;
+ else if ((Q | R) == T)
+return 1;
+ else
+int k = T | Q;
+ Empty EmptyVal;
+ int emptytest = EmptyVal | B;
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: test/clang-tidy/misc-enum-misuse-strict.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-enum-misuse-strict.cpp
@@ -0,0 +1,92 @@
+// RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions: [{key: misc-enum-misuse.StrictMode, value: 1}]}" --
+
+enum A {
+ A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X {
+ X = 8,
+ Y = 16,
+ Z = 4
+};
+// CHECK-MESSAGES: :[[@LINE+1]]:1: warning: enum type seems like a bitmask but possibly contains misspelled number(s) [misc-enum-misuse]
+enum PP {
+ P = 2,
+ Q = 3,
+ R = 4,
+ S = 8,
+ T = 16,
+ U = 31
+};
+
+enum {
+ H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days {
+ Monday,
+ Tuesday,
+ Wednesday,
+ Thursday,
+ Friday,
+ Saturday,
+ Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+ if (P + Q == R)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: right hand side value is not power-of-2 unlike most other values in the enum type
+ else if ((Q | R) == T)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:13: warning: left hand side value is not power-of-2 unlike most other values in the enum type
+ else
+int k = T | Q;
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: right hand side value is not power-of-2 unlike most other values in the enum type
+ unsigned p = R;
+ PP pp = Q;
+ p |= pp;
+ // Line 65 triggers the LINE:17 warning
+ p = A | G;
+ return 0;
+}
+
+int dont_trigger() {
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+
+ unsigned bitflag;
+ enum A aa = B;
+ bitflag = aa | C;
+
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: docs/clang-tidy/checks/misc-enum-misuse.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-enum-misuse.rst
@@ -0,0 +1,67 @@
+.. title:: clang-tidy - misc-enum-misuse
+
+misc-enum-misuse
+
+
+The checker detects various cases when an enum is probably misused (as a bitmask).
+
+1. When "ADD" or "bitwise OR" is used between two enum which come from different
+ types and these types value ranges are not disjoint.
+
+In the following cases you can choose either "Strict" or "Weak" option.
+In "Strict" mode we check if the used EnumConstantDecl type contains almost
+only pow-of-2 numbers.
+We regard the enum as a bitmask if the two
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
aaron.ballman added inline comments.
Comment at: test/clang-tidy/misc-enum-misuse.cpp:3
@@ +2,3 @@
+
+enum Empty {
+};
szepet wrote:
> Could you specify which part of the file seems off? I have run the clang
> format with the same options on testfiles as on the others.
```
enum A { A = 1,
B = 2,
C = 4,
D = 8,
E = 16,
F = 32,
G = 63
};
```
should be:
```
enum A {
A = 1,
B = 2,
C = 4,
D = 8,
E = 16,
F = 32,
G = 63
};
```
is what I was thinking was incorrect, but perhaps clang-format allows such
constructs?
Comment at: test/clang-tidy/misc-enum-misuse.cpp:66
@@ +65,3 @@
+int k = T | Q;
+ return 0;
+ Empty EmptyVal;
Do you intend to have the `return 0;` here?
https://reviews.llvm.org/D22507
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
szepet added inline comments.
Comment at: test/clang-tidy/misc-enum-misuse.cpp:3
@@ +2,3 @@
+
+enum Empty {
+};
Could you specify which part of the file seems off? I have run the clang format
with the same options on testfiles as on the others.
https://reviews.llvm.org/D22507
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
szepet updated this revision to Diff 70017.
szepet marked 11 inline comments as done.
szepet added a comment.
Changes based on comments, fix a cast to dyn_cast bug, description updated
(hopefully it became more clear).
https://reviews.llvm.org/D22507
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/EnumMisuseCheck.cpp
clang-tidy/misc/EnumMisuseCheck.h
clang-tidy/misc/MiscTidyModule.cpp
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-argument-comment.rst
docs/clang-tidy/checks/misc-enum-misuse.rst
test/clang-tidy/misc-enum-misuse-strict.cpp
test/clang-tidy/misc-enum-misuse.cpp
Index: test/clang-tidy/misc-enum-misuse.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-enum-misuse.cpp
@@ -0,0 +1,81 @@
+// RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions: [{key: misc-enum-misuse.StrictMode, value: 0}]}" --
+
+enum Empty {
+};
+
+enum A { A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X { X = 8,
+ Y = 16,
+ Z = 4
+};
+
+enum { P = 2,
+ Q = 3,
+ R = 4,
+ S = 8,
+ T = 16
+};
+
+enum { H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days { Monday,
+Tuesday,
+Wednesday,
+Thursday,
+Friday,
+Saturday,
+Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types [misc-enum-misuse]
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+ unsigned p;
+ p = Q | P;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: left hand side value is not power-of-2 unlike most other values in the enum type
+}
+
+int dont_trigger() {
+ if (A + G == E)
+return 1;
+ else if ((Q | R) == T)
+return 1;
+ else
+int k = T | Q;
+ return 0;
+ Empty EmptyVal;
+ int emptytest = EmptyVal | B;
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: test/clang-tidy/misc-enum-misuse-strict.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-enum-misuse-strict.cpp
@@ -0,0 +1,87 @@
+// RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions: [{key: misc-enum-misuse.StrictMode, value: 1}]}" --
+
+enum A { A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X { X = 8,
+ Y = 16,
+ Z = 4
+};
+// CHECK-MESSAGES: :[[@LINE+1]]:1: warning: enum type seems like a bitmask but possibly contains misspelled number(s) [misc-enum-misuse]
+enum PP { P = 2,
+ Q = 3,
+ R = 4,
+ S = 8,
+ T = 16,
+ U = 31
+};
+
+enum { H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days { Monday,
+Tuesday,
+Wednesday,
+Thursday,
+Friday,
+Saturday,
+Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+ if (P + Q == R)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: right hand side value is not power-of-2 unlike most other values in the enum type
+ else if ((Q | R) == T)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:13: warning: left hand side value is not power-of-2 unlike most other values in the enum type
+ else
+int k = T | Q;
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: right hand side value is not power-of-2 unlike most other values in the enum type
+ unsigned p = R;
+ PP pp = Q;
+ p |= pp;
+ // Line 65 triggers the LINE:17 warning
+ p = A | G;
+ return 0;
+}
+
+int dont_trigger() {
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+
+ unsigned bitflag;
+ enum A aa = B;
+ bitflag = aa | C;
+
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: docs/clang-tidy/checks/misc-enum-misuse.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-enum-misuse.rst
@@ -0,0 +1,67 @@
+.. title:: clang-tidy - misc-enum-misuse
+
+misc-enum-misuse
+
+
+The checker detects various cases when an enum
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
alexfh requested changes to this revision.
This revision now requires changes to proceed.
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:215
@@ +214,3 @@
+"number(s)");
+ diag(RhsExpr->getExprLoc(), "Used here as a bitmask.",
+ DiagnosticIDs::Note);
Diagnostic messages are not full sentences, so they shouldn't start with a
capital letter and end with a period.
Comment at: clang-tidy/misc/EnumMisuseCheck.h:26
@@ +25,3 @@
+ EnumMisuseCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context), IsStrict(Options.get("IsStrict", 1)) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
1. Please move the constructor body to the .cpp file so that the code reading
and storing options are together.
2. Let's use a global flag `StrictMode` (used by one more check currently:
http://clang.llvm.org/extra/clang-tidy/checks/misc-argument-comment.html,
clang-tidy/misc/ArgumentCommentCheck.cpp). It can still be configured
separately for each check, but overall it improves consistency. Also, let's
make it non-strict by default.
Options.get(...)
should change to
StrictMode(Options.getLocalOrGlobal("StrictMode", 0) != 0)
Comment at: docs/clang-tidy/checks/misc-enum-misuse.rst:21
@@ +20,3 @@
+
+ 2. Investigating the right hand side of += or |= operator. (only in "Strict")
+ 3. Check only the enum value side of a | or + operator if one of them is not
Please enclose inline code snippets in backquotes (`+=`, `|=`, etc.). Many
places in this file and in doxygen comments.
Comment at: docs/clang-tidy/checks/misc-enum-misuse.rst:29
@@ +28,3 @@
+
+.. code:: c++
+
Should be `.. code-block:: c++`.
Comment at: docs/clang-tidy/checks/misc-enum-misuse.rst:31
@@ +30,3 @@
+
+//1.
+Enum {A, B, C}
Code block should be indented. Please compile the doc and make sure the result
seems reasonable.
Comment at: docs/clang-tidy/checks/misc-enum-misuse.rst:31
@@ +30,3 @@
+
+//1.
+Enum {A, B, C}
alexfh wrote:
> Code block should be indented. Please compile the doc and make sure the
> result seems reasonable.
> Could you add some descriptions about what 1 stands for here? strict or
> non-strict? please leave a blank between "//" and comment words, the same
> below. Make sure the code here align with code style.
Still not addressed.
Comment at: docs/clang-tidy/checks/misc-enum-misuse.rst:38
@@ +37,3 @@
+flag = A | H; //OK, disjoint value intervalls in the enum types > probably
good use
+flag = B | F; //warning, have common values so they are probably misused
+
> nit: space after //
> here and below.
This is still not addressed.
Comment at: test/clang-tidy/misc-enum-misuse-weak.cpp:2
@@ +1,3 @@
+// RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions:
[{key: misc-enum-misuse.IsStrict, value: 1}]}" --
+
+enum A { A = 1,
The format still seems off.
Comment at: test/clang-tidy/misc-enum-misuse-weak.cpp:66
@@ +65,3 @@
+ p = A | G;
+ //p = G | X;
+ return 0;
Is the commented line needed?
Comment at: test/clang-tidy/misc-enum-misuse.cpp:2
@@ +1,3 @@
+// RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions:
[{key: misc-enum-misuse.IsStrict, value: 0}]}" --
+
+enum Empty {
Doesn't seem to be done: the format is still off.
https://reviews.llvm.org/D22507
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
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 ) {
+
+ // 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
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
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
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
szepet updated this revision to Diff 65966.
szepet marked 12 inline comments as done.
szepet added a comment.
updates based on comments, counter and search functions replaced by std
functions
https://reviews.llvm.org/D22507
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/EnumMisuseCheck.cpp
clang-tidy/misc/EnumMisuseCheck.h
clang-tidy/misc/MiscTidyModule.cpp
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-enum-misuse.rst
test/clang-tidy/misc-enum-misuse-weak.cpp
test/clang-tidy/misc-enum-misuse.cpp
Index: test/clang-tidy/misc-enum-misuse.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-enum-misuse.cpp
@@ -0,0 +1,81 @@
+// RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions: [{key: misc-enum-misuse.IsStrict, value: 0}]}" --
+
+enum Empty {
+};
+
+enum A { A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X { X = 8,
+ Y = 16,
+ Z = 4
+};
+
+enum { P = 2,
+ Q = 3,
+ R = 4,
+ S = 8,
+ T = 16
+};
+
+enum { H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days { Monday,
+Tuesday,
+Wednesday,
+Thursday,
+Friday,
+Saturday,
+Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types [misc-enum-misuse]
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+ unsigned p;
+ p = Q | P;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: left hand side value is not power-of-2 unlike most other values in the enum type
+}
+
+int dont_trigger() {
+ if (A + G == E)
+return 1;
+ else if ((Q | R) == T)
+return 1;
+ else
+int k = T | Q;
+ return 0;
+ Empty EmptyVal;
+ int emptytest = EmptyVal | B;
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: test/clang-tidy/misc-enum-misuse-weak.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-enum-misuse-weak.cpp
@@ -0,0 +1,88 @@
+// RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions: [{key: misc-enum-misuse.IsStrict, value: 1}]}" --
+
+enum A { A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X { X = 8,
+ Y = 16,
+ Z = 4
+};
+// CHECK-MESSAGES: :[[@LINE+1]]:1: warning: enum type seems like a bitfield but possibly contains misspelled number(s) [misc-enum-misuse]
+enum PP { P = 2,
+ Q = 3,
+ R = 4,
+ S = 8,
+ T = 16,
+ U = 31
+};
+
+enum { H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days { Monday,
+Tuesday,
+Wednesday,
+Thursday,
+Friday,
+Saturday,
+Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
+ if (P + Q == R)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: right hand side value is not power-of-2 unlike most other values in the enum type
+ else if ((Q | R) == T)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:13: warning: left hand side value is not power-of-2 unlike most other values in the enum type
+ else
+int k = T | Q;
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: right hand side value is not power-of-2 unlike most other values in the enum type
+ unsigned p = R;
+ PP pp = Q;
+ p |= pp;
+ // Line 60 triggers the LINE:18 warning
+ p = A | G;
+ //p = G | X;
+ return 0;
+}
+
+int dont_trigger() {
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+
+ unsigned bitflag;
+ enum A aa = B;
+ bitflag = aa | C;
+
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: docs/clang-tidy/checks/misc-enum-misuse.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-enum-misuse.rst
@@ -0,0 +1,66 @@
+.. title:: clang-tidy - misc-enum-misuse
+
+misc-enum-misuse
+
+
+The checker detects various cases when an enum is probably misused (as a bitfield).
+ 1. When "add" or "bitwise
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
Prazek added a subscriber: Prazek.
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:19
@@ +18,3 @@
+
+// Stores a min and a max value which describe an interval.
+struct ValueRange {
s/\/\//\/\/\/
In other words, change // to /// (doxygen comment)
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:192
@@ +191,3 @@
+ const auto *LhsConstant =
+ LhsDecExpr ? dyn_cast(LhsDecExpr->getDecl()) : nullptr;
+ const auto *RhsConstant =
If the pointer returned from dyn_cast(LhsDeclExpr->getDecl())
is assumed to be always not null, thn you can change it to cast<> (It will
assert if null instead of returning nullptr)
Comment at: docs/clang-tidy/checks/misc-enum-misuse.rst:29
@@ +28,3 @@
+
+.. code:: c++
+
I think all the code here needs to be indented by one or more to get into
..code:: block.
https://reviews.llvm.org/D22507
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
etienneb added a comment. some nits Comment at: docs/clang-tidy/checks/misc-enum-misuse.rst:37 @@ +36,3 @@ +unsigned flag; +flag = A | H; //OK, disjoint value intervalls in the enum types > probably good use +flag = B | F; //warning, have common values so they are probably misused nit: space after // here and below. Comment at: docs/clang-tidy/checks/misc-enum-misuse.rst:37 @@ +36,3 @@ +unsigned flag; +flag = A | H; //OK, disjoint value intervalls in the enum types > probably good use +flag = B | F; //warning, have common values so they are probably misused etienneb wrote: > nit: space after // > here and below. nit: intervalls (typo) Comment at: test/clang-tidy/misc-enum-misuse-weak.cpp:48 @@ +47,3 @@ +return 1; + // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types [misc-enum-misuse] + if (I | Y) you can remove "[misc-enum-misuse]" from the following line. Only the first occurrence is enough. https://reviews.llvm.org/D22507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
szepet updated this revision to Diff 65310.
szepet added a comment.
full diff submitted
https://reviews.llvm.org/D22507
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/EnumMisuseCheck.cpp
clang-tidy/misc/EnumMisuseCheck.h
clang-tidy/misc/MiscTidyModule.cpp
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-enum-misuse.rst
test/clang-tidy/misc-enum-misuse-weak.cpp
test/clang-tidy/misc-enum-misuse.cpp
Index: test/clang-tidy/misc-enum-misuse.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-enum-misuse.cpp
@@ -0,0 +1,81 @@
+// RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions: [{key: misc-enum-misuse.IsStrict, value: 0}]}" --
+
+enum Empty {
+};
+
+enum A { A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X { X = 8,
+ Y = 16,
+ Z = 4
+};
+
+enum { P = 2,
+ Q = 3,
+ R = 4,
+ S = 8,
+ T = 16
+};
+
+enum { H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days { Monday,
+Tuesday,
+Wednesday,
+Thursday,
+Friday,
+Saturday,
+Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types [misc-enum-misuse]
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types [misc-enum-misuse]
+ unsigned p;
+ p = Q | P;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: left hand side value is not power-of-2 unlike most other values in the enum type [misc-enum-misuse]
+}
+
+int dont_trigger() {
+ if (A + G == E)
+return 1;
+ else if ((Q | R) == T)
+return 1;
+ else
+int k = T | Q;
+ return 0;
+ Empty EmptyVal;
+ int emptytest = EmptyVal | B;
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: test/clang-tidy/misc-enum-misuse-weak.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-enum-misuse-weak.cpp
@@ -0,0 +1,88 @@
+// RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions: [{key: misc-enum-misuse.IsStrict, value: 1}]}" --
+
+enum A { A = 1,
+ B = 2,
+ C = 4,
+ D = 8,
+ E = 16,
+ F = 32,
+ G = 63
+};
+
+enum X { X = 8,
+ Y = 16,
+ Z = 4
+};
+// CHECK-MESSAGES: :[[@LINE+1]]:1: warning: enum type seems like a bitfield but possibly contains misspelled number(s) [misc-enum-misuse]
+enum PP { P = 2,
+ Q = 3,
+ R = 4,
+ S = 8,
+ T = 16,
+ U = 31
+};
+
+enum { H,
+ I,
+ J,
+ K,
+ L
+};
+
+enum Days { Monday,
+Tuesday,
+Wednesday,
+Thursday,
+Friday,
+Saturday,
+Sunday
+};
+
+Days bestDay() {
+ return Friday;
+}
+
+int trigger() {
+ if (bestDay() | A)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types [misc-enum-misuse]
+ if (I | Y)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types [misc-enum-misuse]
+ if (P + Q == R)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: right hand side value is not power-of-2 unlike most other values in the enum type [misc-enum-misuse]
+ else if ((Q | R) == T)
+return 1;
+ // CHECK-MESSAGES: :[[@LINE-2]]:13: warning: left hand side value is not power-of-2 unlike most other values in the enum type [misc-enum-misuse]
+ else
+int k = T | Q;
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: right hand side value is not power-of-2 unlike most other values in the enum type [misc-enum-misuse]
+ unsigned p = R;
+ PP pp = Q;
+ p |= pp;
+ // Line 60 triggers the LINE:18 warning
+ p = A | G;
+ //p = G | X;
+ return 0;
+}
+
+int dont_trigger() {
+ int a = 1, b = 5;
+ int c = a + b;
+ int d = c | H, e = b * a;
+ a = B | C;
+ b = X | Z;
+
+ unsigned bitflag;
+ enum A aa = B;
+ bitflag = aa | C;
+
+ if (Tuesday != Monday + 1 ||
+ Friday - Thursday != 1 ||
+ Sunday + Wednesday == (Sunday | Wednesday))
+return 1;
+ if (H + I + L == 42)
+return 1;
+ return 42;
+}
Index: docs/clang-tidy/checks/misc-enum-misuse.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-enum-misuse.rst
@@ -0,0 +1,66 @@
+.. title:: clang-tidy - misc-enum-misuse
+
+misc-enum-misuse
+
+
+The checker detects various cases when an enum is probably misused (as a bitfield).
+ 1.
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
hokein added a comment.
Seems that you only uploaded the diff part, I only see the updated part of your
patch now, and can't see the whole patch now (The review page says "Context
not available"), could you upload the whole patch again?
Comment at: docs/clang-tidy/checks/misc-enum-misuse.rst:31
@@ +30,3 @@
+
+//1.
+Enum {A, B, C}
Could you add some descriptions about what 1 stands for here? strict or
non-strict? please leave a blank between "//" and comment words, the same
below. Make sure the code here align with code style.
Comment at: docs/clang-tidy/checks/misc-enum-misuse.rst:62
@@ +61,3 @@
+
+ unsigned flag = 0;
+ flag |= E; //ok
extra indentations.
https://reviews.llvm.org/D22507
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
aaron.ballman added a subscriber: aaron.ballman.
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:31
@@ +30,3 @@
+// Stores a min and a max value which describe an interval.
+struct ValueRange {
+ llvm::APSInt MinVal, MaxVal;
I think this class can be replaced by `std::minmax_element()` over the
`EnumDec->enumerators()` and then grabbing the actual values out of the
resulting `std::pair`.
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:59
@@ +58,3 @@
+
+bool isMaxValAllBitSet(const EnumDecl *EnumDec) {
+ for (auto I = EnumDec->enumerator_begin(), E = EnumDec->enumerator_end();
This function doesn't do what is described. It appears to be checking if the
last value in the enumeration has all its bits set, not if the max value has
all the bits set. e.g., it won't check:
```
enum E {
MaxValue = 0x,
First = 0,
Second
};
```
Comment at: clang-tidy/misc/EnumMisuseCheck.h:24
@@ +23,3 @@
+class EnumMisuseCheck : public ClangTidyCheck {
+const bool IsStrict;
+
hokein wrote:
> Put it to private member.
It is a private member already. I think this usage is fine.
https://reviews.llvm.org/D22507
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
szepet marked an inline comment as done. szepet added a comment. https://reviews.llvm.org/D22507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
szepet removed rL LLVM as the repository for this revision.
szepet updated this revision to Diff 64897.
szepet added a comment.
updating patch based on review comments
https://reviews.llvm.org/D22507
Files:
clang-tidy/misc/EnumMisuseCheck.cpp
clang-tidy/misc/EnumMisuseCheck.h
docs/clang-tidy/checks/misc-enum-misuse.rst
test/clang-tidy/misc-enum-misuse-weak.cpp
test/clang-tidy/misc-enum-misuse.cpp
Index: test/clang-tidy/misc-enum-misuse.cpp
===
--- test/clang-tidy/misc-enum-misuse.cpp
+++ test/clang-tidy/misc-enum-misuse.cpp
@@ -1,6 +1,7 @@
// RUN: %check_clang_tidy %s misc-enum-misuse %t -- -config="{CheckOptions: [{key: misc-enum-misuse.IsStrict, value: 0}]}" --
-enum Empty {};
+enum Empty {
+};
enum A { A = 1,
B = 2,
@@ -8,23 +9,27 @@
D = 8,
E = 16,
F = 32,
- G = 63 };
+ G = 63
+};
enum X { X = 8,
Y = 16,
- Z = 4 };
+ Z = 4
+};
-enum {P = 2,
- Q = 3,
- R = 4,
- S = 8,
- T = 16 };
+enum { P = 2,
+ Q = 3,
+ R = 4,
+ S = 8,
+ T = 16
+};
enum { H,
I,
J,
K,
- L };
+ L
+};
enum Days { Monday,
Tuesday,
@@ -32,14 +37,14 @@
Thursday,
Friday,
Saturday,
-Sunday };
+Sunday
+};
Days bestDay() {
return Friday;
}
int trigger() {
-
if (bestDay() | A)
return 1;
// CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types [misc-enum-misuse]
@@ -47,9 +52,8 @@
return 1;
// CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types [misc-enum-misuse]
unsigned p;
- p = Q | P;
+ p = Q | P;
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: left hand side value is not power-of-2 unlike most other values in the enum type [misc-enum-misuse]
-
}
int dont_trigger() {
@@ -74,4 +78,4 @@
if (H + I + L == 42)
return 1;
return 42;
-};
+}
Index: test/clang-tidy/misc-enum-misuse-weak.cpp
===
--- test/clang-tidy/misc-enum-misuse-weak.cpp
+++ test/clang-tidy/misc-enum-misuse-weak.cpp
@@ -6,24 +6,28 @@
D = 8,
E = 16,
F = 32,
- G = 63 };
+ G = 63
+};
enum X { X = 8,
Y = 16,
- Z = 4 };
+ Z = 4
+};
// CHECK-MESSAGES: :[[@LINE+1]]:1: warning: enum type seems like a bitfield but possibly contains misspelled number(s) [misc-enum-misuse]
enum PP { P = 2,
Q = 3,
R = 4,
S = 8,
T = 16,
- U = 31};
+ U = 31
+};
enum { H,
I,
J,
K,
- L };
+ L
+};
enum Days { Monday,
Tuesday,
@@ -31,7 +35,8 @@
Thursday,
Friday,
Saturday,
-Sunday };
+Sunday
+};
Days bestDay() {
return Friday;
@@ -59,7 +64,7 @@
// Line 60 triggers the LINE:18 warning
p = A | G;
//p = G | X;
-return 0;
+ return 0;
}
int dont_trigger() {
@@ -68,7 +73,7 @@
int d = c | H, e = b * a;
a = B | C;
b = X | Z;
-
+
unsigned bitflag;
enum A aa = B;
bitflag = aa | C;
@@ -80,5 +85,4 @@
if (H + I + L == 42)
return 1;
return 42;
-};
-
+}
Index: docs/clang-tidy/checks/misc-enum-misuse.rst
===
--- docs/clang-tidy/checks/misc-enum-misuse.rst
+++ docs/clang-tidy/checks/misc-enum-misuse.rst
@@ -23,3 +23,44 @@
enum val. (only in "Strict")
4. Check both side of | or + operator where the enum values are from the same
enum type.
+
+Examples:
+
+.. code:: c++
+
+//1.
+Enum {A, B, C}
+Enum {D, E, F}
+Enum {G = 10, H = 11, I = 12};
+
+unsigned flag;
+flag = A | H; //OK, disjoint value intervalls in the enum types > probably good use
+flag = B | F; //warning, have common values so they are probably misused
+
+
+
+//2.
+
+Enum Bitfield { A = 0;
+B = 1;
+C = 2;
+D = 4;
+E = 8;
+F = 16;
+G = 31; //OK, real bitfield
+}
+
+Enum AlmostBitfield { AA = 0;
+ BB = 1;
+ CC = 2;
+ DD = 4;
+ EE = 8;
+ FF = 16;
+ GG; //Problem, forgot to initialize
+}
+
+ unsigned flag = 0;
+ flag |= E; //ok
+ flag |= EE; //warning at the decl, and note that it was used here as a bitfield
+
+void f(const string&); // Good: const is not top level.
Index: clang-tidy/misc/EnumMisuseCheck.h
===
--- clang-tidy/misc/EnumMisuseCheck.h
+++ clang-tidy/misc/EnumMisuseCheck.h
@@ -16,18 +16,20 @@
namespace tidy {
namespace misc {
-/// FIXME: Write a short description.
-///
+/// The checker
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
szepet marked 18 inline comments as done. szepet added a comment. https://reviews.llvm.org/D22507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
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 , const llvm::APSInt ) {
+ 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 , const EnumDecl *EnumDec) {
nit: const int x --> int x for all parameters
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:119
@@ +118,3 @@
+ const ValueRange , 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("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");
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
Re: [PATCH] D22507: Clang-tidy - Enum misuse check
Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.
Please mention this check in docs/ReleaseNotes.rst. See pre-4.0 branch versions
as example.
Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:116
@@ +115,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,
Please separate with empty line.
Comment at: test/clang-tidy/misc-enum-misuse-weak.cpp:9
@@ +8,3 @@
+ F = 32,
+ G = 63 };
+
Please put closing }; on next line. Same in other places.
Comment at: test/clang-tidy/misc-enum-misuse-weak.cpp:83
@@ +82,3 @@
+ return 42;
+};
+
Please fix -Wextra-semi. Is next empty line excessive?
Comment at: test/clang-tidy/misc-enum-misuse.cpp:42
@@ +41,3 @@
+int trigger() {
+
+ if (bestDay() | A)
Unnecessary empty line. Same at the end of function.
Comment at: test/clang-tidy/misc-enum-misuse.cpp:77
@@ +76,2 @@
+ return 42;
+};
Please fix -Wextra-semi.
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
