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.ballman wrote:
> 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.
Thanks for the recommendations! As you can see my grammar and vocabulary is a
"bit strange" so I really appreciated the correction!
================
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 "
----------------
aaron.ballman wrote:
> 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.
Here you can see the results on LLVM. (weak options, less false positive)
{F2319113}
Here I have to mention that the last 4 results could be combined into one,
because it`s actually the usage of the same enum in different files. If you
wish I could easily change it.
https://reviews.llvm.org/D22507
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
