This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add bugprone-non-zero-enum-to-bool-conversion check
ClosedPublic

Authored by PiotrZSL on Feb 14 2023, 11:51 AM.

Details

Summary

Detect implicit and explicit conversions of enum to bool,
when enum doesn't have a enumerator with value equal to 0.
In theory such conversion should always return TRUE.

Diff Detail

Event Timeline

PiotrZSL created this revision.Feb 14 2023, 11:51 AM
Herald added a project: Restricted Project. · View Herald Transcript
PiotrZSL updated this revision to Diff 497408.Feb 14 2023, 11:54 AM

Removed "Offers fixes" from list.rst

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/bugprone/EnumToBoolConversionCheck.h
28 ↗(On Diff #497408)

Should language be checked for C++?

clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst
6 ↗(On Diff #497408)

Please synchronize first statement with statement in Release Notes. Please use double back-ticks for language constructs.

49 ↗(On Diff #497408)

Please use single back-ticks for option values. Default value is usually placed after option description.

PiotrZSL updated this revision to Diff 497692.Feb 15 2023, 8:32 AM

Correct copyrights and commit author

PiotrZSL published this revision for review.Feb 15 2023, 8:33 AM

Ready for review

Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2023, 8:33 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
PiotrZSL updated this revision to Diff 497727.Feb 15 2023, 10:14 AM
PiotrZSL marked 3 inline comments as done.

Corrected issues mentioned in review

PiotrZSL planned changes to this revision.Mar 5 2023, 1:20 PM

Documentation to be updated, option need to be changed to list

PiotrZSL updated this revision to Diff 502752.Mar 6 2023, 12:14 PM

Ping, Rebase, Changed Option from Regexp to List, Improved documentation, Removed usage of deprecated API

LegalizeAdulthood resigned from this revision.Mar 29 2023, 8:06 AM

Such a great check, thanks! I have very minor comments, the most debatable one about the name of the check (but no strong opinion).

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
107

I think the name is a bit generic, do we foresee risk of conflict with other similar checks in the future? I wonder if we should narrow it down to something like bugprone-non-zero-enum-to-bool-conversion. WDYT?

clang-tools-extra/clang-tidy/bugprone/EnumToBoolConversionCheck.cpp
23 ↗(On Diff #502752)

Use explicit type

24 ↗(On Diff #502752)

The convention in the LLVM repo is to use traditional operators, please keep consistency.

27 ↗(On Diff #502752)

Use explicit type

72 ↗(On Diff #502752)

Unnecessary empty line

clang-tools-extra/docs/ReleaseNotes.rst
123

Keep alphabetical order.

clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst
12–13 ↗(On Diff #502752)

I can't think of a case where this could happen, might be worth adding an example below.

14 ↗(On Diff #502752)

Nit: Add space between // and NOLINT

38 ↗(On Diff #502752)

Nit: use 2 chars indentation

39 ↗(On Diff #502752)

Use traditional operators for consistency.

41 ↗(On Diff #502752)

won't

41 ↗(On Diff #502752)

Remove?

clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-bool-conversion-cpp11.cpp
9 ↗(On Diff #502752)

Indent with 2 chars instead of 4.

16 ↗(On Diff #502752)

Nit: Add 2 chars indentation to align with the code.

20 ↗(On Diff #502752)

Maybe test also "int", since it's the most common default? Also test one enum class without explicit underlying type.

Thank you for review, I will correct comments in this week.

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
107

I'm fine with name change....

clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst
41 ↗(On Diff #502752)

enums are ints, you can assign them any value by using static casts or when reading data from external message, so thats why in theory, but in this example you right, there is no "in theory"

PiotrZSL planned changes to this revision.Apr 5 2023, 7:50 AM

TODO: Fix review comments, rename check.

PiotrZSL marked 14 inline comments as done.Apr 8 2023, 10:15 AM
PiotrZSL added inline comments.
clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst
12–13 ↗(On Diff #502752)

I thinkg that in STL we got some enums (flags) used as bitmasks... Just wanted to mention that there could be some known false-positives.

PiotrZSL updated this revision to Diff 511904.Apr 8 2023, 10:15 AM
PiotrZSL marked an inline comment as done.
PiotrZSL retitled this revision from [clang-tidy] Add bugprone-enum-to-bool-conversion check to [clang-tidy] Add bugprone-non-zero-enum-to-bool-conversion check.

Review fixes + check rename

carlosgalvezp accepted this revision.Apr 15 2023, 8:13 AM

LGTM, thanks! Would be good to fix the last nit before landing.

clang-tools-extra/clang-tidy/bugprone/NonZeroEnumToBoolConversionCheck.h
18

Nit remove excessive whitespace

This revision is now accepted and ready to land.Apr 15 2023, 8:13 AM
PiotrZSL updated this revision to Diff 513917.Apr 15 2023, 8:46 AM

Fix check description in source file (white spaces, sync it with release notes)

PiotrZSL marked an inline comment as done.Apr 15 2023, 8:48 AM
PiotrZSL updated this revision to Diff 513923.Apr 15 2023, 9:43 AM

Rebase due to broken baseline