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&id=82533#toc
Repository:
rL LLVM
https://reviews.llvm.org/D22507
F
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 readabil
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 diagnos
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
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
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 w
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 = Res
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-
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
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 e
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 ope
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 @@
+}
+stat
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 enum
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 @
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
c
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 o
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.
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
c
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.",
+
szepet added inline comments.
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
aaron.ballm
szepet updated this revision to Diff 68968.
szepet marked 18 inline comments as done.
szepet added a comment.
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
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
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.
Comme
szepet marked 2 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
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/EnumMisuseCh
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)
===
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 misu
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
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
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
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
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-t
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
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;
h
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-
35 matches
Mail list logo
